impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
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. ( )

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

Patch Set 20:


Looks good to me, though I didn't look at the tests very carefully myself. Just some minor
File be/src/service/impala-server.h:
PS20, Line 541: his function 
nit: delete (Obvious that the comment is talking about this function).
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).
PS20, Line 545: option_key
that's not explained. and is it even needed -- why not just use the key from 'option'?
File be/src/service/
PS20, Line 1236: config.__set_level(TQueryOptionLevel::ADVANCED);
does query_option_levels_ not contain all keys? i.e. should line 1233 be a DCHECK?
File be/src/service/query-options.h:
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
PS20, Line 175: QueryOptionLevels&
use pointer

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Csaba Ringhofer <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Laszlo Gaal <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Thu, 23 Nov 2017 00:55:31 +0000
Gerrit-HasComments: Yes

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