Skip to content

fix: preserve input ordering required by limits#23744

Open
jayhan94 wants to merge 4 commits into
apache:mainfrom
jayhan94:fix_limit_sort
Open

fix: preserve input ordering required by limits#23744
jayhan94 wants to merge 4 commits into
apache:mainfrom
jayhan94:fix_limit_sort

Conversation

@jayhan94

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

EnsureRequirements may remove a SortExec when no ordering requirement is visible from its parent.

This is normally valid, but not when the sorted input is consumed by a limit. LIMIT and OFFSET use the input sequence to determine which rows are returned. In the reported case, an intervening ProjectionExec removes the sort key from its output schema, causing its output_ordering() to become None. The optimizer consequently treats the sort below the projection as unnecessary and removes it, producing incorrect query results.

maintains_input_order() only describes whether an operator reorders its surviving rows, while required_input_ordering() requires a concrete ordering expressible using the child schema. Neither property expresses that an operator semantically depends on an existing input ordering.

What changes are included in this PR?

  • Add ExecutionPlan::requires_input_order_preservation(), with a default value of false for each child.
  • Make GlobalLimitExec and LocalLimitExec declare that their input ordering must be preserved.
  • Update sort enforcement to honor this property when deciding whether a descendant SortExec can be removed.
  • Propagate the preservation requirement through order-preserving operators during sort pushdown.
  • Prevent incompatible ordering requirements from being pushed through order-sensitive operators, while still allowing compatible stricter orderings and existing TopK optimizations.
  • Add physical optimizer and SQL logic regression tests covering a projection that hides the sort key below an OFFSET.

Are these changes tested?

Yes.

The regression tests cover both the physical plan and the original SQL behavior. Existing tests also verify that compatible sort pushdown and TopK optimizations are preserved.

The following checks were run:

cargo test -p datafusion --test core_integration physical_optimizer::enforce_sorting::
cargo test -p datafusion-sqllogictest --test sqllogictests -- subquery_sort
cargo test -p datafusion-physical-optimizer
cargo test -p datafusion-physical-plan --lib
cargo fmt --all -- --check
cargo clippy -p datafusion-physical-plan -p datafusion-physical-optimizer \
  -p datafusion --all-targets --all-features --no-deps -- -D warnings

Are there any user-facing changes?

Yes. Queries containing an ordered subquery followed by LIMIT or OFFSET now preserve the ordering needed to select the correct rows, even when an intervening projection hides the sort columns.

This PR also adds a defaulted method to the public ExecutionPlan trait. Existing implementations do not need to be changed, and the change is not breaking.

@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jul 21, 2026
@jayhan94
jayhan94 marked this pull request as ready for review July 23, 2026 06:41
@jayhan94
jayhan94 force-pushed the fix_limit_sort branch 2 times, most recently from 6ce79b4 to eb5a24a Compare July 24, 2026 02:33
@github-actions github-actions Bot added the ffi Changes to the ffi crate label Jul 24, 2026
@jayhan94

Copy link
Copy Markdown
Author

@alamb PTAL🙏

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

Thanks for this PR @jayhan94

/// Unlike [`Self::required_input_ordering`], this property does not require
/// a particular ordering that can be expressed using the child's schema.
/// It only requires that an existing input ordering is not discarded or
/// replaced with an incompatible ordering by physical optimizations. For

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.

This property seems like it is some internal invariant of EnforceDistribution rather than something that describes how the ExecutionPlan behaves

I don't think we should add this to the ExecutionPlan trait

@alamb

alamb commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Isn't LIMIT already handled for subqueries? Perhaps we just need to extend the same logic to handle OFFSET

I don't understand the need to add a new method to ExecutionPlan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate ffi Changes to the ffi crate optimizer Optimizer rules physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Returns the wrong grouped row when an outer GROUP BY consumes a subquery containing ORDER BY ... OFFSET.

2 participants