Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14538 )
Change subject: IMPALA-4400: aggregate runtime filters locally ...................................................................... Patch Set 22: (16 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? Filed IMPALA-9333. I think the benefit is kinda marginal so I set the priority low. 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 Done. I think I got the regressions in formatting. 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 Done 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 Done http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h@168 PS22, Line 168: producer > is_producer Done http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h@233 PS22, Line 233: /// Reader must hold at least one shard lock. The shard bit refers to a previous locking scheme. Updated it to reflect that there's a lock in each PerFilterState. http://gerrit.cloudera.org:8080/#/c/14538/22/be/src/runtime/runtime-filter-bank.h@237 PS22, Line 237: AllocateScratchBloomFilter > AllocateScratch*Filter Done 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 Done 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? Done (and below too) 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 I didn't like any of the options here. I figured I'd try to minimise the size of the diff here, but I don't feel that strongly. 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 instanc Yeah this was wrong. I cleaned up this sentence. I think the second part of the comment makes more sense now. http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@121 PS22, Line 121: globla > typo Done 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 Done http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@292 PS22, Line 292: : > formatting Done http://gerrit.cloudera.org:8080/#/c/14538/22/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@295 PS22, Line 295: : > formatting Done 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 lo I was able to reduce the nested a bit. In the process I realised that multiplying the per-instance profile was a bug, so i fixed that and updated the planner tests. -- 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 23:45:41 +0000 Gerrit-HasComments: Yes
