impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <>
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. (

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

Patch Set 3:

Commit Message:
PS3, Line 25: EXPLAIN_LEVEL=2 and MT_DOP=1.:
> Maybe just make this the example in line 14 and 19?
File shell/
PS3, Line 1430:   options.set_query_options.update(
> Just to confirm: this is command line overriding what's in the config file,
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
File shell/
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.
PS3, Line 39:       print >> sys.stderr, "WARNING: Unable to read configuration file
correctly. " \
> Perhaps: "Ignoring unrecognized config option '%s'"?
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 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.
PS3, Line 65:   Validates some values (False, True, None)
> This only happens for the shell options part of it, yes?
PS3, Line 73:   loaded_options = []
> This is both shell options and impala query options, yeah? Should we make t
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 is still a bit "strange" in my opinion, but I am not sure that rewriting
it should be in the scope of this commit.
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

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Csaba Ringhofer <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Comment-Date: Tue, 10 Oct 2017 20:08:35 +0000
Gerrit-HasComments: Yes

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