impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Date Thu, 28 Sep 2017 18:46:39 GMT
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 <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: Thu, 28 Sep 2017 18:46:39 +0000
Gerrit-HasComments: Yes

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