Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14538 )
Change subject: IMPALA-4400: aggregate runtime filters locally ...................................................................... Patch Set 22: (17 comments) http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/coordinator.cc@371 PS22, Line 371: // TODO: having a shared RuntimeFilterBank between all fragments on a backend Is there a JIRA for this? http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/coordinator.cc@1216 PS22, Line 1216: : Formatting here was correct before, here and several places below (probably a merge conflict issue) http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/coordinator.cc@1217 PS22, Line 1217: if (!IsExecuting() && rpc_params.has_bloom_filter()) { I think you resolved a merge conflict incorrectly here? (a previous version of Fang-Yu's patch had it this way, and you probably rebased onto that verison first, but we changed it) We'll still want to do the 'return' if !IsExecuting() even if its a min-max filter. http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h File be/src/runtime/runtime-filter-bank.h: http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h@167 PS22, Line 167: // nit: formatting http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h@168 PS22, Line 168: producer is_producer http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h@184 PS22, Line 184: locked separately typo http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h@237 PS22, Line 237: AllocateScratchBloomFilter AllocateScratch*Filter http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.cc@69 PS22, Line 69: formatting http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-state.h@63 PS22, Line 63: /// Shared state for Impala's runtime query execution. Used in two contexts: Thanks for doing this! http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/util/bloom-filter.cc@250 PS22, Line 250: DCHECK DCHECK_NE? http://gerrit.cloudera.org:8080/#/c/14538/22/common/thrift/Planner.thrift File common/thrift/Planner.thrift: http://gerrit.cloudera.org:8080/#/c/14538/22/common/thrift/Planner.thrift@64 PS22, Line 64: 12 The numbering here is kinda weird. Maybe worth just fully renumbering? Not a big deal if you prefer to leave as is. http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@110 PS22, Line 110: an instance of I find this wording confusing, since perBackend... is shared across instances. http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@121 PS22, Line 121: globla typo http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@122 PS22, Line 122: is accounted : // are accounted typo http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@292 PS22, Line 292: : formatting http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@295 PS22, Line 295: : formatting http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@496 PS22, Line 496: perHostProfile Not a big deal, but I think you could reduce the code duplication here a lot by just having perHostProfile and totalRuntimeFilterReservation get set based on whether mt_dop == 0, and then all of the builder.appends could just be here once. -- To view, visit http://gerrit.cloudera.org:8080/14538 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iabeeab5eec869ff2197250ad41c1eb5551704acc Gerrit-Change-Number: 14538 Gerrit-PatchSet: 22 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[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-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 27 Jan 2020 20:11:41 +0000 Gerrit-HasComments: Yes
