Skip to content

Fix window output ordering inference and remove redundant sorts - #27439

Draft
blaginin wants to merge 8 commits into
mainfrom
db/window-ordering-baseline
Draft

Fix window output ordering inference and remove redundant sorts#27439
blaginin wants to merge 8 commits into
mainfrom
db/window-ordering-baseline

Conversation

@blaginin

@blaginin blaginin commented Nov 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This should fix:

SET datafusion.execution.target_partitions = 1;
WITH t AS (
  SELECT arrow_cast(x, 'UInt32') AS x
  FROM (VALUES (1), (2), (3)) AS v(x)
)
SELECT SUM(x) OVER (
  ORDER BY x DESC ROWS BETWEEN 1 PRECEDING AND CURRENT ROW
) AS s
FROM t
ORDER BY s ASC;
  • False monotonicity of sliding COUNT
SELECT COUNT(x) OVER (
  ORDER BY x ASC NULLS LAST
  ROWS BETWEEN 1 PRECEDING AND CURRENT ROW
) AS c
FROM (VALUES (1), (2), (CAST(NULL AS INT))) AS t(x)
ORDER BY c ASC;
  • Per-partition ordering incorrectly treated as global ordering
SELECT COUNT(*) OVER (
  PARTITION BY g ORDER BY ts
  ROWS BETWEEN 1 PRECEDING AND CURRENT ROW
) AS c
FROM (VALUES (1, 1), (1, 2), (2, 3)) AS t(g, ts)
ORDER BY c ASC;

and hopefully removes an extra sort:

  • by partition key and ROW_NUMBER when the data is already in that order
  • by partition key and sliding MIN/MAX result
  • for sliding MIN/MAX results that already follow the requested order
  • for ordered MIN/MAX results from frames with FOLLOWING

@blaginin blaginin self-assigned this Nov 14, 2026
@blaginin blaginin added the bug Something isn't working label Nov 14, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation physical-plan Changes to the physical-plan crate labels Nov 14, 2026
@codecov-commenter

codecov-commenter commented Nov 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.31250% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.33%. Comparing base (73e0b03) to head (127cedc).

Files with missing lines Patch % Lines
datafusion/physical-plan/src/windows/mod.rs 95.04% 2 Missing and 4 partials ⚠️
Flag Coverage Δ
all-tests 83.33% <95.31%> (-0.01%) ⬇️
sqllogictests 60.28% <95.31%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@            Coverage Diff             @@
##             main   #27439      +/-   ##
==========================================
- Coverage   83.33%   83.33%   -0.01%     
==========================================
  Files        1054     1054              
  Lines      467862   467853       -9     
  Branches   467862   467853       -9     
==========================================
- Hits       389894   389872      -22     
- Misses      53930    53948      +18     
+ Partials    24038    24033       -5
Files with missing lines Coverage Δ
datafusion/expr/src/udaf.rs 73.80% <ø> (+0.35%) ⬆️
datafusion/physical-expr/src/aggregate.rs 94.18% <100.00%> (+0.39%) ⬆️
datafusion/physical-expr/src/window/aggregate.rs 86.28% <100.00%> (-8.49%) ⬇️
...sion/physical-expr/src/window/sliding_aggregate.rs 76.43% <100.00%> (+3.05%) ⬆️
datafusion/physical-plan/src/windows/mod.rs 92.55% <95.04%> (-0.01%) ⬇️

... and 2 files with indirect coverage changes

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

blaginin
blaginin commented Nov 14, 2026
View reviewed changes
Comment thread datafusion/physical-plan/src/windows/mod.rs
Comment on lines +498 to +511
match (frame.is_ever_expanding(), frame.end_bound.is_unbounded()) {
(true, false) => add_window_ordering(
eq_properties,
growing.clone(),
partition_ordering,
result_never_null,
),
(false, true) => add_window_ordering(
eq_properties,
growing.reverse(),
partition_ordering,
result_never_null,
),
_ => {}

@blaginin blaginin Nov 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@itang06 i think this partially covers #27267 if you want to review this bit. also happy to remove it from here so you can merge your change 🙂

@blaginin

blaginin commented Nov 14, 2026

Copy link
Copy Markdown
Member Author

this fixes everything i want it to fix but i will think about the code until tmr

@github-actions github-actions Bot removed the functions Changes to functions implementation label Nov 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@blaginin blaginin

Labels

bug Something isn't working core Core DataFusion crate documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

2 participants

@blaginin @codecov-commenter