Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 )
Change subject: IMPALA-2181: Add query option levels for display ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h@516 PS7, Line 516: /// Given a query option name this function gets the option level for it to use when : /// displaying from Impala shell and adds the level to the ConfigVariable parameter : /// in numeric format. For values see TQueryOptionLevel enum. > nit: For me, this comment is too complex and tells too much implementation Thx, that sounds better indeed. Done http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc@1228 PS7, Line 1228: const string& option_key > Why do we pass option_key here? It is already there in config, isn't it? In this case it is. But if I relied on that than it would introduce some risk when someone starts to rearrange the part where config is populated as this function can't guarantee that config contains the key every time. To be on the safe side I think it's reasonable to keep the key as a parameter. http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@240 PS7, Line 240: if print_mode == QueryOptionDisplayModes.ALL_OPTIONS: : if len(advanced_options) > 0: : print '\nAdvanced Query Options:' : self._print_option_group(advanced_options) : if len(deprecated_options) > 0: : print '\nDeprecated Query Options:' > nit: this can be simplified as: Done http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@620 PS7, Line 620: ESS : : # Remove any extra spaces surrounding the tokens. > These members are already available in _print_with_set through self. And as Good point! Done -- 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: 8 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: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Nov 2017 20:51:31 +0000 Gerrit-HasComments: Yes
