adriangb commented on code in PR #18644:
URL: https://github.com/apache/datafusion/pull/18644#discussion_r2548545616
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1293,12 +1293,31 @@ impl ExecutionPlan for AggregateExec {
) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
let mut result =
FilterPushdownPropagation::if_any(child_pushdown_result.clone());
+ // If this node tried to pushdown some dynamic filter before, now we
check
+ // if the child accept the filter
if matches!(phase, FilterPushdownPhase::Post) &&
self.dynamic_filter.is_some() {
- let child_accepts_dyn_filter = child_pushdown_result
- .self_filters
- .first()
- .map(|filters| !filters.is_empty())
- .unwrap_or(false);
+ // 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 dyn_filter = self.dynamic_filter.as_ref().unwrap();
+ let child_accepts_dyn_filter = Arc::strong_count(dyn_filter) > 1;
Review Comment:
I'm curious what @LiaCastaneda thinks of this? It seems like quite an
elegant way to say "is anyone using this filter". If no one has a reference to
it it's almost certain no one is using it... We could even make a method like:
```rust
struct DynamicFilterPhysicalExpr {
fn is_used(self: Arc<Self>) -> bool { Arc::strong_count(self) > 1 }
}
```
?
--
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]
