impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Date Wed, 11 Oct 2017 18:42:39 GMT
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 <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: Wed, 11 Oct 2017 18:42:39 +0000
Gerrit-HasComments: Yes

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