You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/09/29 14:33:52 UTC
[PR] Add order-preserving constructors for filter pushdown results [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add order-preserving constructors for filter pushdown results [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #25878:
URL: https://github.com/apache/datafusion/pull/25878#issuecomment-5893363792
I don't think this is worth it
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add order-preserving constructors for filter pushdown results [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb closed pull request #25878: Add order-preserving constructors for filter pushdown results
URL: https://github.com/apache/datafusion/pull/25878
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add order-preserving constructors for filter pushdown results [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25878:
URL: https://github.com/apache/datafusion/pull/25878#issuecomment-5894215209
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25878?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `90.54054%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.57%. Comparing base ([`4a5d580`](https://app.codecov.io/gh/apache/datafusion/commit/4a5d58006e620bf6a175bb578be6652c3b330da9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b96dc7a`](https://app.codecov.io/gh/apache/datafusion/commit/b96dc7ac0286f2b13a97e42fa3bff38e4a5a514d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25878?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-plan/src/projection.rs](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fprojection.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9wcm9qZWN0aW9uLnJz) | 57.14% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/filter\_pushdown.rs](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Ffilter_pushdown.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9maWx0ZXJfcHVzaGRvd24ucnM=) | 92.30% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/datasource/src/file\_scan\_config/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&filepath=datafusion%2Fdatasource%2Fsrc%2Ffile_scan_config%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlL3NyYy9maWxlX3NjYW5fY29uZmlnL21vZC5ycw==) | 80.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/datasource/src/source.rs](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&filepath=datafusion%2Fdatasource%2Fsrc%2Fsource.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlL3NyYy9zb3VyY2UucnM=) | 87.50% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25878?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25878 +/- ##
==========================================
- Coverage 82.58% 82.57% -0.01%
==========================================
Files 1142 1142
Lines 440930 440927 -3
Branches 440930 440927 -3
==========================================
- Hits 364126 364102 -24
- Misses 54801 54815 +14
- Partials 22003 22010 +7
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25878?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org