Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-2956: Filters should be able to target multiple scan nodes ......................................................................
Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/2932/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 148: const auto& id = tnode.node_id; > seems simple to just inline below Done http://gerrit.cloudera.org:8080/#/c/2932/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 512: int target_ndx = filter.planid_to_target_ndx.at(plan_node.node_id); > avoid at() because it can throw Done http://gerrit.cloudera.org:8080/#/c/2932/2/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: Line 368: std::vector<TPlanNodeId> targets; > TRuntimeFilterTargetDesc? Why do you need to put there the entire thing? Besides the desc already has the list of targets. http://gerrit.cloudera.org:8080/#/c/2932/2/fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java File fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java: Line 1994: public List<SlotId> getSlotsWithValueTransfer(SlotId slotId) { > other naming ideas to consider: Done Line 1999: if (targetSid.asInt() >= maxSlotId || slotId.asInt() >= maxSlotId) continue; > you can check srcSlotId outside of the loop Done Line 2514: private int numRegisteredSlots_ = globalState_.descTbl.getMaxSlotId().asInt() + 1; > final Done http://gerrit.cloudera.org:8080/#/c/2932/2/fe/src/main/java/com/cloudera/impala/planner/PlanNode.java File fe/src/main/java/com/cloudera/impala/planner/PlanNode.java: Line 633: for (int i = 0; i < filter.getTargets().size(); ++i) { > consider adding this as a helper in RuntimeFilter, something like Done http://gerrit.cloudera.org:8080/#/c/2932/2/fe/src/main/java/com/cloudera/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/com/cloudera/impala/planner/RuntimeFilterGenerator.java: Line 98: private final List<ScanNode> targets_ = Lists.newArrayList(); > similar suggestion as with the thrift structs: might make sense to package Done Line 105: // Slots from base table tuples that are in the same equivalent classes as the slots > remove references to "equivalent" and "equivalence class" Done Line 130: private boolean hasRemoteTargets_ = false; > This is not true if the filter is produced by a partitioned join? Done Line 183: appliedOnPartitionColumns = > logic seems wrong because appliedOnPartitionColumns starts out as true (i.e Ooops, thanks. Done Line 271: for (SlotId equivSlotId: analyzer.getSlotsWithValueTransfer(slotId)) { > targetSlotId or targetSid Done Line 468: * Registers a runtime filter with a specific tuple id. > ... with a specific target tuple id. Done Line 470: private void registerRuntimeFilter(RuntimeFilter filter, TupleId tupleId) { > tupleId -> targetTid Done Line 486: private void unregisterRuntimeFilter(RuntimeFilter runtimeFilter) { > In this new multi-target world the concept of 'registered' is a little conf Done Line 489: targetTupleIds.add(targetNode.getTupleIds().get(0)); > targetTupleIds.addAll(targetNode.getTupleIds()); Done Line 503: * Currently, runtime filter can only be assigned to HdfsScanNodes. > runtime filters Done Line 519: * the scan node with tuple descriptor 'tid'. > with target tuple id Done Line 521: private Expr computeTargetExpr(RuntimeFilter filter, TupleId tid, Analyzer analyzer) { > tid -> targetTid Done -- To view, visit http://gerrit.cloudera.org:8080/2932 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iad2ce4e579a30616c469312a4e658140d317507b Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
