Skip to content

fix: Support RANGE window frames over binary ORDER BY keys - #24357

Merged
kosiew merged 4 commits into
apache:mainfrom
fornwall:range-window-bytes
Aug 20, 2026
Merged

fix: Support RANGE window frames over binary ORDER BY keys#24357
kosiew merged 4 commits into
apache:mainfrom
fornwall:range-window-bytes

Conversation

@fornwall

@fornwall fornwall commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

A window function over a binary ORDER BY key fails even with no frame clause:

SELECT x, COUNT(*) OVER (ORDER BY x)
FROM (VALUES (arrow_cast('a', 'Binary')), (arrow_cast('b', 'Binary'))) t(x);

Internal error: Cannot run range queries on datatype: Binary.

ORDER BY without a frame defaults to RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW, and extract_window_frame_target_type had no arm for the binary types. LargeBinary, BinaryView, FixedSizeBinary and dictionary-wrapped binary are affected too. Those bounds only need to compare order key values, which binary supports just as Utf8 does.

A finite offset like 1 PRECEDING is different: the bound is computed as current_value - 1, which needs arithmetic on the order key type. Binary has none, and neither do Utf8, Boolean, List and Null -- yet offset frames over Utf8, Boolean and List keys were accepted, and since 54.0.0 the failing arithmetic is swallowed by the overflow handling from #22140, silently collapsing the bound to the partition edge:

SELECT x, COUNT(*) OVER (ORDER BY x RANGE BETWEEN 1 PRECEDING AND CURRENT ROW)
FROM (VALUES ('a'), ('b'), ('c')) t(x) ORDER BY x;

returns the running count 1, 2, 3 -- a silently widened frame -- instead of an error. This PR rejects such frames at planning time instead, matching PostgreSQL.

What changes are included in this PR?

  • Accept the binary types in extract_window_frame_target_type. The existing dictionary arm already recurses into the value type.
  • Reject finite RANGE offsets during planning when the target type has no arithmetic, meaning anything other than the numerics and Interval. The message follows PostgreSQL's RANGE with offset PRECEDING/FOLLOWING is not supported for column type ....
  • Make WindowFrame::free_range() pub, so the check reuses the crate's existing notion of "bounds that are UNBOUNDED or CURRENT ROW".
  • Document the restriction in the window functions guide and the 55.0.0 upgrade guide.

Are these changes tested?

Yes, in window.slt: all four binary types plus dictionary-wrapped binary, ascending and descending, an aggregate and RANK, with duplicate values asserting peer semantics rather than positional counting. Planning errors for offsets over Binary, Utf8, Boolean, List(Int64) and Null, on the end bound as well as the start. Free range frames over Utf8 and Boolean still return results, and Decimal128 keeps its offsets.

The full sqllogictest suite passes.

Are there any user-facing changes?

Yes, one breaking SQL change: RANGE BETWEEN <offset> PRECEDING/FOLLOWING over a Utf8, Binary, Boolean, List or Null ORDER BY key now fails at planning time with RANGE with offset PRECEDING/FOLLOWING is not supported for ORDER BY type .... Over Utf8, Boolean and List keys this replaces the silently widened frames described above; over Null and binary keys such queries already failed at planning time, so only the error message changes. The 55.0.0 upgrade guide entry covers the per-type history and the migration paths: use a ROWS frame to count rows, or state UNBOUNDED PRECEDING / UNBOUNDED FOLLOWING explicitly to compare against the partition edge.

In the other direction, binary order keys now work with free range frames, where they previously raised the internal error above.

WindowFrame::free_range() going from private to pub is additive, so not a breaking Rust API change.


AI usage: Created with Claude Code and Opus 5. I have reviewed the code and made modifications where it made sense. I have also tested this end to end in an internal project.

@github-actions github-actions Bot added documentation Improvements or additions to documentation logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Aug 14, 2026
@fornwall
fornwall force-pushed the range-window-bytes branch 2 times, most recently from ce39143 to 5951847 Compare August 14, 2026 06:50
@github-actions github-actions Bot added the development-process Related to development process of DataFusion label Aug 14, 2026
@fornwall
fornwall force-pushed the range-window-bytes branch 2 times, most recently from cbcb22f to e0663ae Compare August 14, 2026 08:13
Closes apache#24327.

A window with an `ORDER BY` over a binary column and no explicit frame gets
the default `RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW`. Computing
that frame's bounds goes through `extract_window_frame_target_type`, which
had no arm for the binary types, so any window function whose leading
`ORDER BY` key was `Binary`/`LargeBinary`/`BinaryView`/`FixedSizeBinary`
failed with:

    Internal error: Cannot run range queries on datatype: Binary

Binary is orderable, so peer/range comparison is well defined for it in
exactly the same way it already is for `Utf8`. Accept the binary types
alongside the string types.

A finite RANGE offset such as `1 PRECEDING` asks for more than ordering: the
bound is computed as `current_value - 1`, so the order key type must also
support arithmetic. Binary does not, and neither do the `Utf8`, `Boolean`,
`List` and `Null` order keys that `extract_window_frame_target_type` already
accepted. For those, `ScalarValue::sub_checked` failed during execution and
the error was swallowed by the overflow handling from apache#22140, which collapses
a failed bound to the partition edge. So

    SELECT COUNT(*) OVER (ORDER BY x RANGE BETWEEN 1 PRECEDING AND CURRENT ROW)
    FROM (VALUES ('a'), ('b'), ('c')) t(x)

silently returned the running count 1, 2, 3 -- the whole partition -- rather
than an error.

Reject the offset form during planning for every target type that has no
arithmetic. For the string, boolean, list and null order keys this turns a
wrong result into a plan error, which matches PostgreSQL's "RANGE with offset
PRECEDING/FOLLOWING is not supported for column type ...".

`WindowFrame::free_range()` becomes public so that check can be expressed in
the vocabulary the crate already uses for "bounds that need no arithmetic".

Document the restriction in the window functions user guide, and the changed
SQL behavior in the 55.0.0 upgrade guide.

Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
@fornwall
fornwall force-pushed the range-window-bytes branch from e0663ae to 146f516 Compare August 14, 2026 12:47
@codecov-commenter

codecov-commenter commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.26%. Comparing base (00eba79) to head (143907d).
⚠️ Report is 53 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24357      +/-   ##
==========================================
+ Coverage   81.19%   81.26%   +0.06%     
==========================================
  Files        1110     1112       +2     
  Lines      388616   391585    +2969     
  Branches   388616   391585    +2969     
==========================================
+ Hits       315527   318203    +2676     
- Misses      54509    54675     +166     
- Partials    18580    18707     +127     

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

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fornwall,

Thanks for working on this. The change looks good overall. I left one small, non-blocking suggestion to make the regression coverage a little more explicit.

Comment thread datafusion/sqllogictest/test_files/window.slt
@fornwall
fornwall requested a review from kosiew August 18, 2026 21:57

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fornwall, thanks for the follow-up. The added Dictionary(Int32, Binary) coverage in window.slt exercises RANGE BETWEEN 1 PRECEDING AND CURRENT ROW and confirms that the dictionary type is unwrapped before the finite-offset arithmetic rejection. That covers the case I was looking for.

I also reran the relevant validation and didn't find any new issues. Looks good to me, thanks!

@kosiew
kosiew added this pull request to the merge queue Aug 19, 2026
@kosiew

kosiew commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🚀
Thanks @fornwall

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Aug 19, 2026
@fornwall

Copy link
Copy Markdown
Contributor Author

The PR was removed from the queue. But that seems like some general CI issue, not specific to this PR: https://github.com/apache/datafusion/actions/runs/32227729624/job/95990834191

@fornwall
fornwall requested a review from kosiew August 19, 2026 09:22
@kosiew
kosiew added this pull request to the merge queue Aug 20, 2026
Merged via the queue into apache:main with commit f1f0449 Aug 20, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Related to development process of DataFusion documentation Improvements or additions to documentation logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Window functions fail when ORDER BY uses a binary column

3 participants