Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-2956: Filters should be able to target multiple scan nodes ......................................................................
Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/2932/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 499: f->pending_count = filter.is_broadcast_join ? 1 : num_hosts; > this will also get set to 1 even for a local-only filter, and probably show I don't think we should change anything here. pending_count indicates how many backends to wait for in order to mark the filter complete and it shouldn't depend on the filter target types (local/remote). I believe we should update the output in the runtime profile. Line 499: f->pending_count = filter.is_broadcast_join ? 1 : num_hosts; > could you take a close look at what's going on here? we need to make sure w I took a look at it. The output is clearly misleading for the LOCAL case, i.e. it indicates that pending_count is non-zero. I didn't change this code but I modified the output of runtime filter table to show as "N/A". Line 2034: vector<unordered_set<int32_t>> target_fragment_instance_idxs; > if these are all fragment instance indices, why not just keep them in a sin Done Line 2044: DCHECK(!state->desc.has_local_targets || state->desc.has_remote_targets) > dcheck(state->desc.has_remote_targets) Done http://gerrit.cloudera.org:8080/#/c/2932/4/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: Line 370: std::vector<bool> is_local_target; > comment what it's indexed by (presumably mirrors targets, but good to spell Created a struct that represents a target and moved the information there. Line 373: // Index into fragment_instance_states_ for target fragment instances. > same here. also, who owns memory? Done http://gerrit.cloudera.org:8080/#/c/2932/4/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: Line 88: // produced the runtime filter > and false == it's a remote target, i assume? Correct Line 104: 4: required map<Types.TPlanNodeId, i32> planid_to_target_ndx > why not simply put the node id into TRuntimeFilterTargetDesc? Done http://gerrit.cloudera.org:8080/#/c/2932/4/fe/src/main/java/com/cloudera/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/com/cloudera/impala/planner/RuntimeFilterGenerator.java: Line 136: // Indicates if 'target' is in the same fragment as the join that produces the > 'node' Done Line 188: public void finalize() { finalized_ = true; } > markFinalized() 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: 4 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-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
