Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20612 )
Change subject: IMPALA-3825: Delegate runtime filter aggregation to some executors ...................................................................... Patch Set 13: (15 comments) http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/coordinator.cc@478 PS10, Line 478: int pending_count = > It looks a bit strange that pending count is set at line 432, then we set i Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/query-state.cc@438 PS10, Line 438: his filter regist > The new address compare function could be used here. Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h File be/src/runtime/runtime-filter-bank.h: http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h@100 PS10, Line 100: /// Filters are aggregated, first locally in this RuntimeFilterBank, if there are multiple > Can you mention the new aggregation mechanism in the class comment? Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h@196 PS10, Line 196: > nit: has completed? Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h@197 PS10, Line 197: _t, std::unique_ > nit: that has not completed? Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h@198 PS10, Line 198: const boost::unordered_map< > Can you mention RUNTIME_FILTER_WAIT_TIME_MS handling in the commit message? Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h@199 PS10, Line 199: ; > nit: doesn't happen? Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h@227 PS10, Line 227: xpec > nit: holds Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.h@246 PS10, Line 246: > nit: number of? Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.cc@139 PS10, Line 139: AllocateScratchBloomFilter > Would it be hard to allocate the filter only when we first need it? Filter aggregation can happen sometime later after all fragment start. I'm worried that if memory is not preallocated since start, other fragment can get greedy in scaling their memory reservation beyond initial reservation, and the remaining memory will not be sufficient to allocate an intermediary bloom filter then. This is why I preallocated them here. http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.cc@722 PS10, Line 722: result_filter->SetFilter(B > Shouldn't we mark the filter as always true? Agree. Done. http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.cc@756 PS10, Line 756: > nit: cancelled Done http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/util/network-util.h File be/src/util/network-util.h: http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/util/network-util.h@107 PS10, Line 107: KrpcAddressEqual > naming: I think that KrpcAddressEquals would be clearer Done http://gerrit.cloudera.org:8080/#/c/20612/10/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/20612/10/common/thrift/ImpalaInternalService.thrift@54 PS10, Line 54: desi > nit: "that" not needed Done http://gerrit.cloudera.org:8080/#/c/20612/10/common/thrift/ImpalaInternalService.thrift@54 PS10, Line 54: gator. > nit: aggregator? Done -- To view, visit http://gerrit.cloudera.org:8080/20612 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11d38ed0f223d6e5b32a19ebe725af7738ee4ab0 Gerrit-Change-Number: 20612 Gerrit-PatchSet: 13 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Wed, 13 Dec 2023 01:17:34 +0000 Gerrit-HasComments: Yes
