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 21: (8 comments) http://gerrit.cloudera.org:8080/#/c/8447/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8447/20//COMMIT_MSG@14 PS20, Line 14: the SET SQL s > I think that should say "the SET SQL statement" or similar, since SET can Done 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: ets the optio > nit: delete (Obvious that the comment is talking about this function). Done http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@544 PS20, Line 544: query option. > if that's modified, you should use ConfigVariable *. We prefer to use poin Done http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@545 PS20, Line 545: lToConfig( > that's not explained. and is it even needed -- why not just use the key fro I didn't want to make this function dependent on whether the key field of the ConfigVariable was set by the caller or not. I found it safer this way. I'll mention this in the comment. 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: > does query_option_levels_ not contain all keys? i.e. should line 1233 be a In fact, a DCHECK is enough here. (Changing this to a DCHECK actually caught a bug: Where support_start_over was added to the options map the name should have been added with uppercase letters.) 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: Maps query > comments shouldn't talk about current code vs. older code. They should just Done http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h@175 PS20, Line 175: QueryOptionLevels* > use pointer Done http://gerrit.cloudera.org:8080/#/c/8447/20/common/thrift/beeswax.thrift File common/thrift/beeswax.thrift: http://gerrit.cloudera.org:8080/#/c/8447/20/common/thrift/beeswax.thrift@98 PS20, Line 98: > maybe we should be super explicit here and say: 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: 21 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: Thu, 23 Nov 2017 05:56:49 +0000 Gerrit-HasComments: Yes
