fix: remap projected predicates before WindowTopN - #25069
Closed
discord9 wants to merge 1 commit into
Closed
Conversation
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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]makesval <= 1refer to index 2. With inputpk=1, val=10, the original plan returns no rows; the rule mistakes that index forrn, 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_exprhelper. 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:
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-optimizercargo test -p datafusion --test core_integration physical_optimizer::window_topn::— 20 passedcargo fmt --all --checkcargo clippy --workspace --all-targets --all-features -- -D warningsThe 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.