Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 )
Change subject: IMPALA-2181: Add query option levels for display ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1221 PS2, Line 1221: default_query_options_ Why pass in 'default_query_options_'? Isn't it already visible in AddOptionLevelToConfigDescription()? http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1224 PS2, Line 1224: ConfigVariable support_start_over; : support_start_over.__set_key("support_start_over"); : support_start_over.__set_value("false"); : default_configs_.push_back(support_start_over); Shouldn't you call AddOptionLevelToConfigDescription() with 'support_start_over' as well? http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@106 PS2, Line 106: is not It is safer to use "!=" here. "is not" works for small integers with the CPython interpreter but it is implementation specific behavior and might be changed in the future. http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@107 PS2, Line 107: return "" nit: maybe we should be more defensive and return "" as well if parsed_description[0] != "OptionLevel". http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@127 PS2, Line 127: PRINT_REGULAR_OPTIONS_ONLY = 1 : PRINT_ALL_OPTIONS = 2 nit: why not use a bool flag instead? http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@225 PS2, Line 225: = nit: remove spaces around '=' http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@250 PS2, Line 250: that 'then'? http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@266 PS2, Line 266: default_options[option_name] Why not use 'option_value'? http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@263 PS2, Line 263: elif level == self.QUERY_OPTION_ADVANCED: : advanced_options[option_name] = option_value : else: : advanced_options[option_name] = default_options[option_name] How about this instead? else: advanced_options[option_name] = option_value -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 02 Nov 2017 17:32:23 +0000 Gerrit-HasComments: Yes
