impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Date Thu, 23 Nov 2017 00:55:31 GMT
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
......................................................................


Patch Set 20:

(6 comments)

Looks good to me, though I didn't look at the tests very carefully myself. Just some minor
comments.

http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@541
PS20, Line 541: his function 
nit: delete (Obvious that the comment is talking about this function).


http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@544
PS20, Line 544: beeswax::ConfigVariable& option
if that's modified, you should use ConfigVariable *.  We prefer to use pointers rather than
references for things that get modified, so that it's clear at the callsite.  (i.e. only use
const references).


http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@545
PS20, Line 545: option_key
that's not explained. and is it even needed -- why not just use the key from 'option'?


http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.cc@1236
PS20, Line 1236: config.__set_level(TQueryOptionLevel::ADVANCED);
does query_option_levels_ not contain all keys? i.e. should line 1233 be a DCHECK?


http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h
File be/src/service/query-options.h:

http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h@34
PS20, Line 34: Introduced
comments shouldn't talk about current code vs. older code. They should just talk about the
code. So, reword this:

Maps from query option name to option level used ...

or similar


http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h@175
PS20, Line 175: QueryOptionLevels&
use pointer



-- 
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: 20
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: Thu, 23 Nov 2017 00:55:31 +0000
Gerrit-HasComments: Yes

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