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