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 4: (10 comments) Thanks! I think this is much clearer than the previous iteration. All of my comments are either nits or requests to add a few more comments. http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@686 PS4, Line 686: tmp_set = {} Add a comment: # Use a temporary to avoid changing set_query_options during iteration. (An alternative is to use del, but use .items() instead of .iteritems()) http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1330 PS4, Line 1330: if __name__ == "__main__": Perhaps we need a big comment: """ There are two types of options: shell options and query_options. You can set these in the shell itself, which overrides settings on the command line, which override the defaults in impala_shell_config_defaults.py. Query options have no defaults within the impala-shell, but they do have defaults on the server. """ http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1350 PS4, Line 1350: # default options loaded in from impala_shell_config_defaults.py Let's take the time to update this comment by disambiguating "shell options" vs "query options" here? http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1354 PS4, Line 1354: impala_shell_defaults.update(loaded_shell_options) I think "impala_query_options_default" is empty, but this assymetry between impala_shell_defaults and query_options is weird. It's weird that you're updating impala_shell_defaults, rather than updating "shell_options". http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1437 PS4, Line 1437: query_options.update( Add comment: Override query_options with those specified on the command line. http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@36 PS4, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename): Please document config_filename. I'm unclear how it's used. http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@47 PS4, Line 47: # if the option is not set to either true or false, use the default Do we need to log about the ignored value here? http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@36 PS4, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename): : """Converts some values based on their type in default_options : """ : if default_options[option] in [True, False]: : # validate the option if it can only be a boolean value : # the only choice for these options is true or false : if value.lower() == "true": : return True : elif value.lower() == 'false': : return False : else: : # if the option is not set to either true or false, use the default : return default_options[option] : elif value.lower() == "none": : return None : elif option.lower() == "config_file": : return config_filename This function is mixing 2-space indent and 4-space indent. Please clean up. http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@164 PS4, Line 164: "Only specify this as a option in the commandline." s/as a option/as an option/ http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@183 PS4, Line 183: help="Sets default query options." Add: "May be specified multiple times." Unless it's clear from the usage? -- 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: 4 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: Wed, 11 Oct 2017 18:42:39 +0000 Gerrit-HasComments: Yes
