LiaCastaneda commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3721873258
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -2097,27 +2151,12 @@ impl ExecutionPlan for AggregateExec {
if phase == FilterPushdownPhase::Post
&& let Some(dyn_filter) = &self.dynamic_filter
{
- // let child_accepts_dyn_filter = child_pushdown_result
- // .self_filters
- // .first()
- // .map(|filters| {
- // assert_eq_or_internal_err!(
- // filters.len(),
- // 1,
- // "Aggregate only pushdown one self dynamic filter"
- // );
- // let filter = filters.get(0).unwrap(); // Asserted above
- // Ok(matches!(filter.discriminant, PushedDown::Yes))
- // })
- // .unwrap_or_else(|| internal_err!("The length of self
filters equals to the number of child of this ExecutionPlan, so it must be
1"))?;
-
- // HACK: The above snippet should be used, however, now the child
reply
- // `PushDown::No` can indicate they're not able to push down
row-level
- // filter, but still keep the filter for statistics pruning.
- // So here, we try to use ref count to determine if the dynamic
filter
- // has actually be pushed down.
- // Issue: <https://github.com/apache/datafusion/issues/18856>
- let child_accepts_dyn_filter = Arc::strong_count(dyn_filter) > 1;
Review Comment:
Even if the rewrite to use `apply_expressions` looks like it adds a bunch of
code, I think this is a nice use case for it, relying on the strong count of
the dynamic filter's inner/outer `Arc` has caused several headaches before.
Maybe we could do something similar in `HashJoinExec` to determine whether the
dynamic filter has a consumer? Right now that's done via `is_used`, which
relies on strong count. IIRC it's only used to check for consumers, so maybe we
could remove `is_used` entirely in favor of this approach?
https://github.com/apache/datafusion/blob/3e3a92de29ed3d454e72c7bade6328508b6098c6/datafusion/physical-plan/src/joins/hash_join/exec.rs#L1385
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
