impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabor Kaszab (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Date Tue, 07 Nov 2017 20:51:31 GMT
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 <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Nov 2017 20:51:31 +0000
Gerrit-HasComments: Yes

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