Skip to content

[branch-56] fix: retain finer sorts above merges (backport #27060) - #27331

Merged
alamb merged 1 commit into
apache:branch-56from
geoffreyclaude:backport/27060-branch-56
Nov 11, 2026
Merged

alamb merged 1 commit into
apache:branch-56from
geoffreyclaude:backport/27060-branch-56

Conversation

@geoffreyclaude

@geoffreyclaude geoffreyclaude commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

A finer ordering pushed below SortPreservingMergeExec is lost when the merge keeps fewer keys. This produces incorrectly ordered results and can make a window query fail plan validation. Downstream testing still reproduces the failure in the 56 line.

The original fix merged after branch-56 was cut and is needed before the DataFusion 56 release candidate.

What changes are included in this PR?

Cherry-pick 3f4a37fbabdcb07705db6341268e6c0b34da0d34 from #27060. Keep the finer parent sort above the merge, together with the original regression tests.

The limit.slt conflict was append context from an unrelated test absent on branch-56; only the original #26162 regression block is added.

What is the testing strategy for this PR?

Carries the original limit.slt checks for ordered output, a window, an ordered aggregate and a UNION, plus the physical-plan regression for a merge without a fetch.

cargo fmt --all, both targeted physical-plan tests and the limit.slt checks passed. Reverting the optimizer change makes the new non-fetched-merge regression fail.

Are there any user-facing changes?

Queries retain their requested ordering above sort-preserving merges. No public API changes.

This PR was prepared with AI assistance.

…apache#26162) (apache#27060)

## Which issue does this PR close?
- Closes apache#26162.

## Rationale for this change
In apache#26162, running `ORDER BY b, a` over a multi-partition subquery with
`ORDER BY b LIMIT n` produces incorrect ordering on `a`.

Pushing `[b, a]` down removed the outer `SortExec`, but SPM only merges
on `[b]`. As a result, `a` was not sorted in the final output.

In the discussion, @jayzhan211 suggested two possible approaches:
declining pushdown for SPM with fetch, or rewriting SPM's expressions
when pushing through it.

I chose `Ok(None)` (declining pushdown) because:
- The subquery did not explicitly request the parent's sort. When a
limit is present, forcing that sort on the entire dataset across
partitions before a limit is unnecessary work.
- Since `pushdown_requirement_to_children` only adds requirements to
children, I think a logic to rewrite SPM adds extra complexity.

## What changes are included in this PR?
- Return `Ok(None)` from `pushdown_requirement_to_children` when `plan`
is `SortPreservingMergeExec` and has a `fetch`.
- Add an SLT regression query in `limit.slt`.

## What is the testing strategy for this PR?
- query on a partitioned table in `limit.slt` reproducing apache#26162.

## Are there any user-facing changes?
Yes, bug fix: queries like `ORDER BY b, a` over a multi-partition
subquery with `ORDER BY b LIMIT n` now return correctly ordered results
on `a`. No API changes.

---------

Co-authored-by: Jay Zhan <[email protected]>

(cherry picked from commit 3f4a37f)
@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Nov 11, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.28%. Comparing base (964f925) to head (59e3754).

Flag Coverage Δ
all-tests 83.28% <100.00%> (-0.01%) ⬇️
sqllogictests 60.84% <100.00%> (-0.01%) ⬇️

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

@@              Coverage Diff              @@
##           branch-56   #27331      +/-   ##
=============================================
- Coverage      83.29%   83.28%   -0.01%     
=============================================
  Files           1170     1170              
  Lines         475484   475469      -15     
  Branches      475484   475469      -15     
=============================================
- Hits          396035   396018      -17     
- Misses         55476    55480       +4     
+ Partials       23973    23971       -2     
Files with missing lines Coverage Δ
...sure_requirements/enforce_sorting/sort_pushdown.rs 90.60% <100.00%> (+0.36%) ⬆️

... and 8 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.

@alamb alamb 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.

Thank you @geoffreyclaude .

@alamb
alamb merged commit 3f197fe into apache:branch-56 Nov 11, 2026
42 checks passed
alamb added a commit that referenced this pull request Nov 12, 2026
## Which issue does this PR close?

- Part of #24461

## Rationale for this change

Several PRs were backported to `branch-56` after the changelog was
generated in #27098, so they
are missing from `dev/changelog/56.0.0.md`.

## What changes are included in this PR?

Regenerates the changelog with `dev/release/generate-changelog.py 55.0.0
branch-56 56.0.0`. This adds entries for:

- #27243
- #27204
- #27331
- #27369
- #27368
- #27098

and updates the commit count and credits.

## What is the testing strategy for this PR?

N/A (changelog only)

## Are there any user-facing changes?

No

Co-authored-by: Claude Fable 5.2 <[email protected]>
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 sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants