Skip to content

fix: validate trailing ORDER BY keys in free RANGE frames - #25518

Open
viirya wants to merge 1 commit into
apache:mainfrom
viirya:codex/fix-free-range-order-types
Open

viirya wants to merge 1 commit into
apache:mainfrom
viirya:codex/fix-free-range-order-types

Conversation

@viirya

@viirya viirya commented Sep 19, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

  • Closes #.

Rationale for this change

A free RANGE window frame determines peers using every ORDER BY
expression. However, when the first expression had an offset target type, the
analyzer did not validate the remaining expressions.

Consequently, an unsupported trailing Map expression could reach execution
and rows with different map values could incorrectly be treated as peers.

What changes are included in this PR?

Validate all trailing ORDER BY expressions when the first expression
establishes the RANGE target type.

This preserves the existing behavior that accepts a List as the first
ORDER BY expression, while rejecting unsupported trailing expressions.

A SQL logic regression test for ORDER BY int_col, map_col is included.

What is the testing strategy for this PR?

  • Added a regression test in window.slt.
  • Verified that the old code returns incorrect peer counts while the fixed
    code reports a planning error.
  • Verified existing first-List and supported multi-key cases.
  • Ran cargo fmt --all -- --check.
  • Ran cargo clippy -p datafusion-optimizer --all-targets --all-features -- -D warnings.

Are there any user-facing changes?

Yes. Free RANGE frames with an unsupported trailing ORDER BY type are now
rejected during planning instead of potentially producing incorrect results.

There are no public API changes.

@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Sep 19, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.38%. Comparing base (710c748) to head (1d62c26).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25518      +/-   ##
==========================================
- Coverage   82.38%   82.38%   -0.01%     
==========================================
  Files        1138     1138              
  Lines      434313   434313              
  Branches   434313   434313              
==========================================
- Hits       357805   357804       -1     
+ Misses      54876    54874       -2     
- Partials    21632    21635       +3     

☔ 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.

@viirya
viirya requested a review from sunchao September 20, 2026 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants