You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "discord9 (via GitHub)" <gi...@apache.org> on 2026/09/08 11:49:34 UTC
[PR] fix: remap projected predicates before WindowTopN [datafusion]
discord9 opened a new pull request, #25069:
URL: https://github.com/apache/datafusion/pull/25069
## Which issue does this PR close?
No linked issue. Independently reproduced physical-rule correctness fix.
## Rationale for this change
WindowTopN accepts projections between a filter and its window, but interprets the filter's column index in the window's schema without remapping it. A projection can place a data column at the original ranking-column index, causing the rule to remove a real data filter and return incorrect rows.
For example, window output `[pk, val, rn]` projected to `[pk, rn, val]` makes `val <= 1` refer to index 2. With input `pk=1, val=10`, the original plan returns no rows; the rule mistakes that index for `rn`, inserts top-1, removes the filter, and returns the row.
This is reproduced through the direct physical-rule API with WindowTopN enabled. The option defaults to false. The equivalent SQL tested retains the correct filter after earlier normalization; this PR does not claim a default-SQL failure.
## What changes are included in this PR?
Remap a local copy of the predicate through intermediate projections, in filter-to-window order, using the existing positional `unproject_expr` helper. Only then identify a supported ranking-column bound. Unsupported remaps or computed predicates leave the original plan unchanged.
Intermediate reconstruction remains unchanged. No new mapping framework, public API, or configuration is introduced. This is independent of the TopK aggregation window-boundary fix in #25066.
## What is the testing strategy for this PR?
Native execution coverage checks:
- a reordered data predicate that must return no rows;
- a reordered data predicate retaining two rows, rather than truncating them to top-1;
- a ranking predicate through two reordered/aliased projections that still optimizes and returns the correct top-K;
- a computed ranking expression that must remain unsupported and preserve its result.
Removing only predicate remapping makes the first regression return one row instead of zero. The source was restored byte-exactly, and all 20 WindowTopN tests passed afterward.
Verified locally:
- `cargo check -p datafusion-physical-optimizer`
- `cargo test -p datafusion --test core_integration physical_optimizer::window_topn::` — 20 passed
- `cargo fmt --all --check`
- `cargo clippy --workspace --all-targets --all-features -- -D warnings`
The full workspace runtime test suite was not run.
## Are there any user-facing changes?
Correct results when the enabled physical WindowTopN rule encounters projected predicates. No public API or configuration changes.
--
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] fix: remap projected predicates before WindowTopN [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 closed pull request #25069: fix: remap projected predicates before WindowTopN
URL: https://github.com/apache/datafusion/pull/25069
--
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] fix: remap projected predicates before WindowTopN [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25069:
URL: https://github.com/apache/datafusion/pull/25069#issuecomment-5587153607
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25069?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 `72.72727%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.74%. Comparing base ([`b39cb4a`](https://app.codecov.io/gh/apache/datafusion/commit/b39cb4a549bfa7b5619cbd16d060a2737b12c6cd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0ee0956`](https://app.codecov.io/gh/apache/datafusion/commit/0ee0956a3ba8fde6bdc4a4ba05980075b55c61a4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25069?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-optimizer/src/window\_topn.rs](https://app.codecov.io/gh/apache/datafusion/pull/25069?src=pr&el=tree&filepath=datafusion%2Fphysical-optimizer%2Fsrc%2Fwindow_topn.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1vcHRpbWl6ZXIvc3JjL3dpbmRvd190b3BuLnJz) | 72.72% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25069?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 #25069 +/- ##
==========================================
- Coverage 81.74% 81.74% -0.01%
==========================================
Files 1128 1128
Lines 416644 416652 +8
Branches 416644 416652 +8
==========================================
- Hits 340585 340584 -1
- Misses 55999 56004 +5
- Partials 20060 20064 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25069?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