impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Date Thu, 12 Oct 2017 19:41:12 GMT
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 <csringhofer@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Oct 2017 19:41:12 +0000
Gerrit-HasComments: Yes

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