Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-2956: Filters should be able to target multiple scan nodes ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/2932/4/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 151: if (state->query_options().runtime_filter_mode == TRuntimeFilterMode::LOCAL && > why do we need this check now? Because now a filter may have both local and remote targets. Even in LOCAL mode the filter will be assigned to all the target scan nodes. So, we need to make sure that the node that is the remove target isn't waiting for the filter if the mode is LOCAL. http://gerrit.cloudera.org:8080/#/c/2932/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 492: if (filter_mode_ == TRuntimeFilterMode::LOCAL && !filter.has_local_targets) { > if a filter doesn't have any remote targets, there's also no need to regist Previously, we would still register in the coordinator a filter with a local target if the mode was GLOBAL. Do we wish to change that behavior? Line 505: // If this is a broadcast join with non-local targets, only build and publish it > "with only non-local" Done Line 518: int target_ndx = it->second; > const TRuntimeFilterTargetDesc& target = ..; Done Line 2081: if (state->is_local_target[i]) continue; > we shouldn't see local targets here To preserve the existing behavior, a filter is registered in the coordinator if the mode is GLOBAL irrespective of what type of targets it has (local only, remote only, mix). Hence, we need to make sure here that we don't send the filter to local targets. http://gerrit.cloudera.org:8080/#/c/2932/4/be/src/runtime/runtime-filter.cc File be/src/runtime/runtime-filter.cc: Line 98: bool has_local_targets = false; > i'd still stick with the singular, because it only needs to have one (local Why do you say it only needs to have one? A filter can have many local and many remote targets at the same time. Is this what you meant? -- 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
