discord9 commented on code in PR #22926:
URL: https://github.com/apache/datafusion/pull/22926#discussion_r3670830910
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -2067,11 +2067,17 @@ impl ExecutionPlan for AggregateExec {
.map(|i| Column::new(output_schema.field(i).name(), i))
.collect();
- // Analyze each filter separately to determine if it can be pushed down
- let mut safe_filters = Vec::new();
- let mut unsafe_filters = Vec::new();
+ // Analyze each filter separately to determine if it can be pushed
down.
+ // The order of `parent_filters` must be preserved: the filter pushdown
+ // optimizer maps child results back to the original parent filters by
+ // position.
+ let child = self.children()[0];
+ let mut child_desc =
ChildFilterDescription::from_child(&parent_filters, child)?;
Review Comment:
Done — I switched AggregateExec to
`ChildFilterDescription::from_child_with_allowed_indices`, using the grouping
output indices that are present in every grouping set. While applying this, I
also tightened the empty-input case: global aggregates and grouping sets
containing `()` now mark all parent filters unsupported, since they can emit a
row from empty input. Added regression coverage for aggregate/input name
collisions and constant predicates.
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -2067,11 +2067,17 @@ impl ExecutionPlan for AggregateExec {
.map(|i| Column::new(output_schema.field(i).name(), i))
.collect();
- // Analyze each filter separately to determine if it can be pushed down
- let mut safe_filters = Vec::new();
- let mut unsafe_filters = Vec::new();
+ // Analyze each filter separately to determine if it can be pushed
down.
+ // The order of `parent_filters` must be preserved: the filter pushdown
+ // optimizer maps child results back to the original parent filters by
+ // position.
+ let child = self.children()[0];
+ let mut child_desc =
ChildFilterDescription::from_child(&parent_filters, child)?;
- for filter in parent_filters {
+ for (filter, child_parent_filter) in parent_filters
Review Comment:
Added the runtime `assert_eq_or_internal_err!` in `push_down_filters` before
positional mapping. It checks each child descriptor against
`parent_filtered.len()`, so a malformed implementation returns an internal
error in release builds rather than silently misassociating filters. The
focused filter-pushdown module suite passes (54 tests).
--
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]
