adriangb opened a new pull request, #25878: URL: https://github.com/apache/datafusion/pull/25878
## Which issue does this PR close? - Related to #22883. ## Rationale for this change The filter pushdown optimizer maps each node's `PushedDown` results back to the parent filters by position. If a node returns its results in a different order, for example after sorting filters by selectivity, the lengths still match. The optimizer then removes the wrong filter above the scan, and the query silently returns incorrect results. Nothing on `main` reorders these results today. The hazard becomes real as adaptive predicate evaluation (#22883) adds new places that decide per filter. The docs on `ExecutionPlan::handle_child_pushdown_result` also currently say the order "need not match", which pushes implementers toward exactly this bug. This PR makes the order-preserving path the easy one: a node that builds its result with these helpers can't return results in the wrong order. It is purely additive. ## What changes are included in this PR? New constructors in `datafusion/physical-plan/src/filter_pushdown.rs`: - `FilterPushdownPropagation::from_filters(&filters, |f| PushedDown)`: builds the result by mapping over the input filters. - `FilterPushdownPropagation::all_supported(&child_pushdown_result)`: for nodes that handle whatever their children could not, such as `FilterExec`, `SortExec` and `UnionExec`. - `FilterPushdownPropagation::map_node(|node| ...)`: for nodes that delegate pushdown to an inner component and wrap the updated node it returns (`DataSourceExec` → `DataSource` → `FileSource`). - `ChildFilterDescription::from_filters(&parent_filters, |f| PushedDownPredicate)`: a public way to rewrite pushed-down filters, as `ProjectionExec` does. Before this, the only way to do that was a crate-private struct literal. All in-tree call sites now use these helpers. After this PR, no in-tree code calls `with_parent_pushdown_result` or builds `FilterPushdownPropagation` / `ChildFilterDescription` as a struct literal. `with_parent_pushdown_result` stays, with a doc note pointing to the new constructors. The `handle_child_pushdown_result` docs now state that results must be in input order, and that this constrains only the reported results, not the order in which a node evaluates the filters it absorbs. ## What is the testing strategy for this PR? The behaviour doesn't change, so existing tests cover the migrated call sites. I ran: - the `datafusion-physical-plan`, `datafusion-datasource` and `datafusion-datasource-parquet` unit tests; - the `core_integration` `physical_optimizer` tests; - the full `sqllogictests` suite. `from_filters` has a doc test. ## Are there any user-facing changes? There are new public helper methods, and no breaking changes. A follow-up could deprecate `with_parent_pushdown_result` and make `FilterPushdownPropagation::filters` private, so that a positional result can't be built by hand at all. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DiM8EzJ1Uownn7jufdxYpr -- 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]
