Repository navigation
[branch-56] fix: retain finer sorts above merges (backport #27060) - #27331
Open
geoffreyclaude wants to merge 1 commit into
Open
geoffreyclaude wants to merge 1 commit into
geoffreyclaude wants to merge 1 commit into
Conversation
…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 <jayzhan211@gmail.com> (cherry picked from commit 3f4a37f)
github-actions
Bot
added
optimizer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
branch-56.56.0.0(Oct/Nov 2026) #24461.Rationale for this change
A finer ordering pushed below
SortPreservingMergeExecis 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-56was cut and is needed before the DataFusion 56 release candidate.What changes are included in this PR?
Cherry-pick
3f4a37fbabdcb07705db6341268e6c0b34da0d34from #27060. Keep the finer parent sort above the merge, together with the original regression tests.The
limit.sltconflict was append context from an unrelated test absent onbranch-56; only the original #26162 regression block is added.What is the testing strategy for this PR?
Carries the original
limit.sltchecks 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 thelimit.sltchecks 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.