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 4: (10 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: Done 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: Thanks for the nice comment, I made some small changes. 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 Done 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. I think it is less weird now with the long comment at the beginning. Query option defaults come from the server, so the script does not know them yet. > It's weird that you're updating impala_shell_defaults, rather than > updating "shell_options". I do not know why it was done this way originally. http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1437 PS4, Line 1437: query_options.update( > Add comment: Done 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. Done 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? Done 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. Done 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/ Done 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? 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: 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: Thu, 12 Oct 2017 19:41:12 +0000 Gerrit-HasComments: Yes
