fix: Capture global ORDER BY requirement under ScalarSubqueryExec root#23146
Closed
sgrebnov wants to merge 5 commits into
Closed
fix: Capture global ORDER BY requirement under ScalarSubqueryExec root#23146sgrebnov wants to merge 5 commits into
sgrebnov wants to merge 5 commits into
Conversation
require_top_ordering_helper bailed on ScalarSubqueryExec because it has multiple children (main input + subquery plans), so the rule stamped an empty OutputRequirementExec (order_by=[], dist_by=Unspecified) at the root and never recorded the query's global ORDER BY requirement. Descend through child 0 (the order-transparent main input: maintains_input_order()[0] == true, no required input ordering) to find and protect the top SortExec, reattaching the subquery children unchanged.
sgrebnov
changed the title
…ery-output-requirements
sgrebnov
changed the title
sgrebnov
marked this pull request as ready for review
July 12, 2026 20:33
sgrebnov
marked this pull request as draft
July 12, 2026 20:49
…bqueryExec root EXPLAIN VERBOSE pins the 'physical_plan after OutputRequirements' step: the OutputRequirementExec carrying the query's global ORDER BY is placed below the ScalarSubqueryExec root. Without the fix the rule stamps an empty order_by=[], dist_by=Unspecified requirement at the root and this snapshot fails.
Drop the pass-pinning EXPLAIN VERBOSE block from subquery.slt: SQL over built-in sources cannot produce wrong results from this defect (the top SortExec and the requirement-independent merge backstops in EnsureRequirements always restore the global order), so the slt could only ever assert on intermediate pass output while pinning the full pass list. Instead add full_pipeline_keeps_row_order_under_scalar_subquery_root: it runs the complete default physical optimizer pipeline over a ScalarSubqueryExec root whose ordering comes from a SortPreservingMergeExec over a two-partition ordered source (the shape federated planners produce, with no SortExec backstop) and executes the plan. Without the fix the empty root requirement lets the pipeline drop the merge and the rows come back partition-interleaved.
…ing tests Rename the tests to name the unit each one exercises: the rule traversal (require_top_ordering_descends_through_scalar_subquery) and the end-to-end ordering guarantee (scalar_subquery_root_preserves_global_ordering_end_to_end). Rebuild the end-to-end test around record_batch!, drop the redundant plan-shape assertion and the unneeded optimizer config knobs (verified: the executed-row-order check alone still catches the regression), and describe the expected behavior rather than the failure mode. Replace unwrap() with descriptive expect().
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23146 +/- ##
=======================================
Coverage ? 80.66%
=======================================
Files ? 1087
Lines ? 367420
Branches ? 367420
=======================================
Hits ? 296364
Misses ? 53397
Partials ? 17659 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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?
After the DataFusion 53 → 54 upgrade, queries with a
ScalarSubquerystoppedrespecting the global
ORDER BYand returned unordered results.Rationale for this change
In add mode,
OutputRequirementswalks the plan viarequire_top_ordering_helperto capture the globalORDER BYas anOutputRequirementExec. The helper bails on any node withchildren.len() != 1.ScalarSubqueryExechas multiple children (child 0 = order-transparent main input, the rest = uncorrelated subquery plans),so when it is the root the rule stops immediately and stamps an empty
OutputRequirementExec(order_by=[], dist_by=Unspecified)at the top — losing the ordering requirement.ScalarSubqueryExecis order-transparent on child 0 (maintains_input_order()[0]== true, no required input ordering), so the search should descend through it like any other single-child order-preserving operator.What changes are included in this PR?
require_top_ordering_helpernow special-casesScalarSubqueryExec: it descends through child 0 to find and wrap the topSortExec/SortPreservingMergeExec, reattaching the subquery children unchanged.Are these changes tested?
Yes — two tests in
datafusion/core/tests/physical_optimizer/output_requirements.rs:Rule level (
require_top_ordering_descends_through_scalar_subquery): asserts theOutputRequirementExeccarrying the ordering is placed below theScalarSubqueryExec; without the fix it lands empty at the root.End to end (
scalar_subquery_root_preserves_global_ordering_end_to_end): runs the full default optimizer pipeline over aScalarSubqueryExecroot whose ordering comes from aSortPreservingMergeExecover a two-partition ordered source (the shape federated/custom planners produce, with noSortExecbackstop), then executes the plan and checks the rows. Verified by toggling the fix:1, 3, 5, 7, 2, 4, 6, 8— merge dropped, rows partition-interleaved1, 2, 3, 4, 5, 6, 7, 8— globally orderedExisting
subquery.slt/ TPC-H snapshots are unchanged. I was unable to construct a SQL query that triggers the bug — over built-in sources the globalORDER BYis always preserved regardless of the missing requirement. It only surfaces when a custom physical planner supplies a plan shaped like the end-to-end test: aSortPreservingMergeExecover already-sorted partitions, with noSortExecabove it.Are there any user-facing changes?
No.