impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Date Tue, 10 Oct 2017 20:08:35 GMT
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038
)

Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8038/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8038/3//COMMIT_MSG@25
PS3, Line 25: EXPLAIN_LEVEL=2 and MT_DOP=1.:
> Maybe just make this the example in line 14 and 19?
Done


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py@1430
PS3, Line 1430:   options.set_query_options.update(
> Just to confirm: this is command line overriding what's in the config file,
Done


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py@1431
PS3, Line 1431:      [(k.upper(),v) for k,v in parse_variables(options.query_options).items()])
> nit: I believe pep8 style has a space after comma
Done


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py
File shell/option_parser.py:

http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@33
PS3, Line 33: def validate_and_convert_options(loaded_options, default_options):
> Let's be clear that these are impala_shell options and not impala_query opt
I did a bit of refactoring, I think that it is more clear now. Please comment if I should
work on it more.


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@39
PS3, Line 39:       print >> sys.stderr, "WARNING: Unable to read configuration file
correctly. " \
> Perhaps: "Ignoring unrecognized config option '%s'"?
Done


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@47
PS3, Line 47:         loaded_options[i] = (option, True)
> It's weird that we're modifying loaded_options here. 
The cause of the runtime error is fixed now, see impala_shell.py line 686-693.

Note that query options only work when the shell is connected to impalad and this was the
reason why MT_DOP was not accepted here.


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@65
PS3, Line 65:   Validates some values (False, True, None)
> This only happens for the shell options part of it, yes?
Yes.


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@73
PS3, Line 73:   loaded_options = []
> This is both shell options and impala query options, yeah? Should we make t
Done


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@79
PS3, Line 79:     loaded_options.append( ("set_query_options",
> It took me longer than I care to admit to understand what's going on here. 
I have changed some of this, I hope that it is less messy now. Some of the option handling
code in impala_shell.py is still a bit "strange" in my opinion, but I am not sure that rewriting
it should be in the scope of this commit.


http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@179
PS3, Line 179:                          " KEY starts with an alphabetic character and"
> This is really about "key" being a valid query option, right? I think you c
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 20:08:35 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message