Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC ...................................................................... Patch Set 20: Code-Review+1 (4 comments) I'll reach out to Michael and see if he wants to do a pass, otherwise I think that this can be +2ed once you address my last few comments. Thanks again for all the work on this! http://gerrit.cloudera.org:8080/#/c/13882/20/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/13882/20/be/src/runtime/coordinator.cc@1113 PS20, Line 1113: DCHECK(!params->bloom_filter().always_false()); not really necessary http://gerrit.cloudera.org:8080/#/c/13882/20/be/src/runtime/coordinator.cc@1126 PS20, Line 1126: if I think you'll want to make this an 'else if' with the 'if' above http://gerrit.cloudera.org:8080/#/c/13882/20/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/13882/20/be/src/runtime/runtime-filter-bank.cc@244 PS20, Line 244: Status status = bloom_filter->Init( There's a bug here - if we fail to get the sidecar above then bloom_filter will be nullptr, so we won't want to call Init() on it http://gerrit.cloudera.org:8080/#/c/13882/19/be/src/util/min-max-filter.h File be/src/util/min-max-filter.h: http://gerrit.cloudera.org:8080/#/c/13882/19/be/src/util/min-max-filter.h@22 PS19, Line 22: #include "impala-ir/impala-ir-functions.h" > Thanks for catching this! I found that we do not need 'data_stream_service' This is getting rather nit-picky of me, but just for your understanding: the goal isn't to have the smallest number of includes that will allow things to compile, the goal is to include exactly those things that are used in the file. Clearly, there are things from data_stream_service that are used in this file now, eg. MinMaxFilterPB. Presumably you don't need to include it for things to compile because its already included in on of the other headers thats included here. To be clear, I'm not saying that you need to go back through this whole patch and make sure your includes are exactly correct. If it compiles as is then that's fine. The most important things is to not include things that aren't used, since that can lead to unnecessary re-compilation in some cases. -- To view, visit http://gerrit.cloudera.org:8080/13882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a Gerrit-Change-Number: 13882 Gerrit-PatchSet: 20 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Wed, 18 Sep 2019 19:22:02 +0000 Gerrit-HasComments: Yes
