Skip to content

fix: remap projected predicates before WindowTopN - #25069

Closed
discord9 wants to merge 1 commit into
apache:mainfrom
discord9:fix/window-topn-projection-remap
Closed

fix: remap projected predicates before WindowTopN#25069
discord9 wants to merge 1 commit into
apache:mainfrom
discord9:fix/window-topn-projection-remap

Conversation

@discord9

@discord9 discord9 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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.

Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate labels Sep 8, 2026
@discord9 discord9 closed this Sep 8, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.74%. Comparing base (b39cb4a) to head (0ee0956).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/physical-optimizer/src/window_topn.rs 72.72% 1 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants