Lars Volker 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 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@686 PS7, Line 686: tmp_set "_set" implies this is a set, but it's actually a dict. I think "new_query_options" might be a better name. http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@1332 PS7, Line 1332: There are two types of options: shell options and query_options. Both can be set in nit: missing article http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@28 PS7, Line 28: # EXPLAIN_LEVEL=2 are these case sensitive? Otherwise I'd opt for consistency with the previous section. http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@36 PS7, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename): : """Converts some values based on their type in default_options : : Setting 'config_filename' in the config file would have no effect, : so its original value is kept. From looking at the signature and the comment I have no idea what this method does. Can you clarify it so it makes sense without further context? The comment also should explain what the return value is. http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@42 PS7, Line 42: if default_options[option] in [True, False]: How about "if type(...) is bool:" ? http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@58 PS7, Line 58: return config_filename What if none of the cases match? http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@66 PS7, Line 66: filtered and some values are converted (False, True, None). Can you explain what exactly gets converted into what? Strings into Python values? http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@72 PS7, Line 72: Returns a pair of dictionaries (shell_options, query_options) Can you add a comment explaining what the keys and values of those dictionaries are? http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@91 PS7, Line 91: upper Is this needed? If so, can you add a comment explaining why? http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@167 PS7, Line 167: "The following sections are used: [impala], " I think we should mention that the section titles are case sensitive -- 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: 7 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-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Tue, 17 Oct 2017 23:08:54 +0000 Gerrit-HasComments: Yes
