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 <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: Tue, 10 Oct 2017 20:08:35 +0000 Gerrit-HasComments: Yes
