Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/24657 )
Change subject: IMPALA-13534: Implement runtime filters on CTEs ...................................................................... Patch Set 22: Code-Review+1 (3 comments) This looks good to me. Couple small things that could be follow-ups. http://gerrit.cloudera.org:8080/#/c/24657/22/be/src/exec/cte-consumer-node.cc File be/src/exec/cte-consumer-node.cc: http://gerrit.cloudera.org:8080/#/c/24657/22/be/src/exec/cte-consumer-node.cc@255 PS22, Line 255: if (!filter_ctxs_.empty()) { : if (!filters_waited_) { : filters_waited_ = true; : WaitForRuntimeFilters(state, filter_ctxs_); : } : FilterRowBatch(output_batch); : } It would be nice to avoid the copy for the is_passthrough_ == false branch for rows that will be eliminated by the runtime filter. That's not a blocker for this change. Down the road, accumulating a full row batch before returning would be nice. That would require multiple GetNext() calls to LocalExchanger, so that conflicts with our memory lifetimes for LocalExchanger right now. Does CTE consumer ever have conjuncts? http://gerrit.cloudera.org:8080/#/c/24657/22/be/src/exec/cte-consumer-node.cc@285 PS22, Line 285: for (const FilterContext& ctx : filter_ctxs_) { : if (ctx.expr_eval != nullptr) ctx.expr_eval->Close(state); : } https://github.com/apache/impala/commit/8f6fdc0f3910503556fc088cc4ef306ac5e96009 added a mechanism to specify if a runtime filter is effective to display in the runtime profile. That logic currently only looks at scan nodes, but I think we'll want to extend it to this. That could be a separate follow-up change. http://gerrit.cloudera.org:8080/#/c/24657/22/be/src/exec/cte-consumer-node.cc@339 PS22, Line 339: void CTEConsumerNode::CheckFiltersEffectiveness() noexcept { : for (int i = 0; i < filter_stats_.size(); ++i) { : LocalFilterStats& stats = filter_stats_[i]; : const RuntimeFilter* filter = filter_ctxs_[i].filter; : double reject_ratio = stats.rejected / static_cast<double>(stats.considered); : if (filter->AlwaysTrue() || reject_ratio < FLAGS_min_filter_reject_ratio) { : stats.enabled_for_row = false; : } : } : } We're duplicating code from HdfsScanner, but I don't see an easy way to avoid it. We want to have per-thread structures for mt_dop=0, so we can't just move it to the ExecNode. We can revisit it later. -- To view, visit http://gerrit.cloudera.org:8080/24657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic877fb590187826f828da6a27bf274465c381e8e Gerrit-Change-Number: 24657 Gerrit-PatchSet: 22 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Wed, 26 Aug 2026 17:36:40 +0000 Gerrit-HasComments: Yes
