Repository navigation
refactor: one rule for the equivalence properties of a merge of partitions - #26643
Draft
jayzhan211 wants to merge 1 commit into
Draft
jayzhan211 wants to merge 1 commit into
jayzhan211 wants to merge 1 commit into
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
kumarUjjawal
pushed a commit
to kumarUjjawal/datafusion
that referenced
this pull request
Oct 23, 2026
…tions (apache#26654) ## Which issue does this PR close? - Closes apache#26646. ## Rationale for this change When a sorted input with more than one partition goes through `SortPreservingMergeExec`, the planner treats every ordering of the input as still holding after the merge. Only the merge key ordering actually holds. The other orderings are true within each partition, but not across them. Because of this, the planner can remove a sort that is still needed, and the query returns rows out of order. With two files that are each sorted by `a` and `b` and `target_partitions = 2`: ```sql SELECT * FROM (SELECT a, b FROM t ORDER BY a LIMIT 10) ORDER BY b; ``` the plan has no sort for the outer `ORDER BY b`, and the rows come back as `(1, 10), (1, 1), (2, 20), (3, 2)`. ## What changes are included in this PR? - `SortPreservingMergeExec::compute_properties` clears the input orderings when there is more than one input partition, before adding the merge key ordering. This matches what `CoalescePartitionsExec` and `RepartitionExec` already do. With a single input partition nothing is merged, so the input orderings are kept as before. The preserve-order `RepartitionExec` case mentioned in the issue is not covered here. Draft apache#26643 moves this logic into `EquivalenceProperties::merged`. If that lands first, I'll update this PR to use `.merged(input_partitions, false)` followed by `add_ordering`. ## What is the testing strategy for this PR? - New sqllogictest case at the end of `order.slt`, using the reproduction from the issue. It checks both the query result and the physical plan, which must keep the `SortExec` above the `SortPreservingMergeExec`. It fails on `main` with the wrong row order from the issue, and passes with this change. - The existing sqllogictests and the `datafusion-physical-plan` / `datafusion-physical-optimizer` unit tests pass with no changes to expected plans. ## Are there any user-facing changes? Queries like the one above now return correctly ordered results. No public API changes.
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?
Rationale for this change
Operators that combine rows from several input partitions must forget the facts that hold only inside each
partition: constants whose value may differ between partitions, and, unless the operator keeps order, the input's
orderings. Today this rule is written out in four places, and the copies disagree when the input has a single
partition:
RepartitionExecEnforceDistributionSortPreservingMergeExecCoalescePartitionsExecWith one input partition, both
SortPreservingMergeExecandCoalescePartitionsExecpass the input streamthrough, so nothing they drop has stopped being true.
What changes are included in this PR?
EquivalenceProperties::merged(input_partitions, keeps_order): with one input partition everything is kept;otherwise per-partition constants are dropped, and orderings too unless
keeps_order.What is the testing strategy for this PR?
merged: one partition against two, with and withoutkeeps_order.CoalescePartitionsExecover one sorted partition reports that ordering (it fails before thischange) and over two does not.
Are there any user-facing changes?
SortPreservingMergeExecandCoalescePartitionsExecover a single input partition keep the facts of theirinput, so the planner may skip a sort it does not need. Results do not change. New public method
EquivalenceProperties::merged.