Philip Zeyliger 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)
The UI feels good to me.
As a UI improvement, I think "impala-shell --help" should mention that it's
possible to write a .impalarc file, and perhaps have a sample file.
There are three types of key-value pairs we keep track of: query options,
impala shell options, and variables. I find the code already very confusing
about how these three states are managed across defaults, config files, and
command line options. I think making that clearer is worthwhile.
I was able to trigger a few bugs:
$impala-shell.sh
Starting Impala Shell without Kerberos authentication
Error connecting: TTransportException, Could not connect to
philip-dev.gce.cloudera.com:21000
MT_DOP is not supported for the impalad being connected to, ignoring.
Traceback (most recent call last):
File "/home/philip/src/impala/shell/impala_shell.py", line 1448, in <module>
shell = ImpalaShell(options)
File "/home/philip/src/impala/shell/impala_shell.py", line 195, in __init__
self.do_connect(options.impalad)
File "/home/philip/src/impala/shell/impala_shell.py", line 686, in do_connect
for set_option in self.set_query_options:
RuntimeError: dictionary changed size during iteration
$impala-shell.sh
WARNING: Unable to read configuration file correctly. Check formatting:
'live_progress'
Starting Impala Shell without Kerberos authentication
Traceback (most recent call last):
File "/home/philip/src/impala/shell/impala_shell.py", line 1432, in <module>
options.set_query_options.update(
AttributeError: Values instance has no attribute 'set_query_options'
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?
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,
yes? I think it's right; we should make sure we test it.
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
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
options. I think we should rename the method as well as the argument names to
make that more obvious.
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'"?
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.
Are we intentionally passing the unrecognized options through?
I think it's poor form to both modify an argument in place and to return it. I
would prefer you accumulating a "ret = []" kind of variable and returning that,
to make this clearer.
Also, I tested it and it broke:
$impala-shell.sh
WARNING: Unable to read configuration file correctly. Check formatting:
'live_progress'
Starting Impala Shell without Kerberos authentication
Error connecting: TTransportException, Could not connect to
philip-dev.gce.cloudera.com:21000
MT_DOP is not supported for the impalad being connected to, ignoring.
Traceback (most recent call last):
File "/home/philip/src/impala/shell/impala_shell.py", line 1448, in <module>
shell = ImpalaShell(options)
File "/home/philip/src/impala/shell/impala_shell.py", line 195, in __init__
self.do_connect(options.impalad)
File "/home/philip/src/impala/shell/impala_shell.py", line 686, in do_connect
for set_option in self.set_query_options:
RuntimeError: dictionary changed size during iteration
$cat ~/.impalarc
[impala]
live_progress=true
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
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?
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 that
clearer?
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
get it now: Impala query options get written into options.set_query_options
eventually. I think it's worthy of more explanation in the pydoc.
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 can
remove lines 179-180, and simply mention that you can see valid query options
with "SET".
--
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 <csringho...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 18:46:39 +0000
Gerrit-HasComments: Yes
