fix: restore input ordering a SortMergeJoinExec requires after lowering - #2066
Closed
andygrove wants to merge 1 commit into
Closed
fix: restore input ordering a SortMergeJoinExec requires after lowering#2066andygrove wants to merge 1 commit into
andygrove wants to merge 1 commit into
Conversation
Lowering a plan into stages rewrites nodes, and a rewrite can drop an ordering its parent depends on. Promoting a `SortMergeJoinExec` to a broadcast `HashJoinExec(CollectLeft)` produces a join that does not preserve input order, so a parent `SortMergeJoinExec` reading that output merged unsorted input. A merge join given unsorted input does not error: it stops matching once the cursors diverge, silently dropping rows and returning a wrong answer. Re-establish any hard input ordering a rewritten child no longer carries. Sorts are only added, never removed, so an input that already satisfies its requirement is left untouched. Also validate the planned stages: every `SortMergeJoinExec` input must satisfy the join's required ordering. An unsorted merge-join input is always a bug, and failing loudly is better than returning wrong results. The check reports no false positives across the full SF1 TPC-DS gate under both `prefer_hash_join=true` and `false`. TPC-DS q4 and q78 now match single-process DataFusion and join the correctness gate, taking it from 86 to 88 queries. q78 was previously filed as "distributed LIMIT drops rows": the wrong answer does not in fact depend on `ORDER BY` or `LIMIT` and reproduces with neither.
Member
Author
|
Moving this to draft while I think about this more. Maybe the current broadcast optimization for SMJ is using the wrong approach. |
andygrove
marked this pull request as ready for review
July 16, 2026 17:40
Member
Author
Let's go ahead and review this. It fixes the correctness issue. I'll keep investigating a better long term solution. |
avantgardnerio
approved these changes
Jul 16, 2026
avantgardnerio
left a comment
Contributor
There was a problem hiding this comment.
Would an extra pass of EnforceSorting be a robust alternative?
Member
Author
Good idea... I will open a separate PR with this approach so we can compare |
andygrove
marked this pull request as draft
July 16, 2026 23:02
Member
Author
|
On second thoughts, the SMJ "broadcast" optimization was just a bad idea. I created #2072 to remove it. |
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?
Closes #2065.
Also resolves the q4 / q78 half of #2046 (the set-operation half, q38 / q87, is
a separate DataFusion-side bug handled in #2064).
Rationale for this change
Lowering a plan into stages rewrites nodes, and a rewrite can drop an ordering
its parent depends on. Ballista's broadcast promotion turns a
SortMergeJoinExecinto aHashJoinExec(CollectLeft), which does not preserveinput order — but nothing re-establishes that order, so a parent
SortMergeJoinExecends up merging unsorted input.A merge join given unsorted input does not error. It stops matching once the
merge cursors diverge, so rows are silently dropped and the query returns a
wrong answer.
In the q78 plan the outer
SortMergeJoinExechad aSortExecon its rightinput and nothing on its left, because the inner SMJ that used to supply that
ordering had been replaced by a broadcast hash join:
Disabling the promotion isolated the cause —
broadcast_join_threshold_bytes=0or
broadcast_sort_merge_join_enabled=falseboth restore the correct answer.This is distinct from #1055, which concerns which join types are safe to
broadcast (already guarded by
collect_left_broadcast_safe). Here the join typeis fine; the ordering contract is what breaks.
What changes are included in this PR?
restore_required_input_ordering: after lowering rewrites a node's children,re-establish any
Hardinput ordering a child no longer carries. Sorts areonly added, never removed, so an input that already satisfies its requirement
is untouched.
validate_sort_merge_join_orderings: verify everySortMergeJoinExecin theplanned stages has inputs satisfying the join's required ordering, so this
failure mode surfaces as an error instead of wrong results.
Softrequirements are deliberately ignored — they are opportunistic and anoperator works without them.
Verification
SF1, 1 scheduler / 1 executor,
target_partitions=16:Full SF1 TPC-DS gate: 88/88 verified against single-process DataFusion,
under both
prefer_hash_join=falseandprefer_hash_join=true. The newvalidation reports 0 false positives across all 88 queries under both
settings, which is why it is a hard error rather than a debug assertion.
cargo test --workspace,cargo clippy --all-targets --workspace -- -D warnings, andcargo fmt --allare clean.Note on the q78 framing
q78 was originally reported under #2046 as "distributed LIMIT drops rows". That
framing is a red herring: the wrong answer does not depend on
ORDER BYorLIMITand reproduces with neither. Wrapping the query incount(*)changesthe plan enough to hide the bug, which is what made it look limit-related.
Are there any user-facing changes?
Yes — queries where a broadcast-promoted join feeds a sort-merge join now
return correct results instead of silently dropping rows. A plan that would hit
this now fails with an explicit error rather than producing a wrong answer. No
API changes.