Skip to content

fix: preserve required ordering in limit pushdown - #24231

Open
Himanshu-2005-code wants to merge 4 commits into
apache:mainfrom
Himanshu-2005-code:fix-limit-required-ordering
Open

fix: preserve required ordering in limit pushdown#24231
Himanshu-2005-code wants to merge 4 commits into
apache:mainfrom
Himanshu-2005-code:fix-limit-required-ordering

Conversation

@Himanshu-2005-code

@Himanshu-2005-code Himanshu-2005-code commented Aug 10, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

Queries using ORDER BY ... LIMIT can return incorrect results after the physical plan is rewritten by limit or sort pushdown.

When an order-sensitive limit is recreated during optimization, the information that the limit depends on a particular ordering can be lost. If the plan is optimized again later, the limit may then be pushed down without preserving that ordering, allowing the underlying scan to return rows in a different order.

This can cause ORDER BY ... LIMIT queries to return different or incorrect rows.

What changes are included in this PR?

  • Preserve required_ordering when GlobalLimitExec and LocalLimitExec nodes are recreated during limit pushdown.
  • Preserve the same ordering metadata for fallback limits created by sort pushdown.
  • Add regression tests covering reinserted order-sensitive limits.

Are these changes tested?

Yes.

  • cargo check -p datafusion-physical-optimizer
  • cargo clippy -p datafusion-physical-optimizer --all-targets -- -D warnings
  • cargo fmt --all -- --check
  • cargo test -p datafusion-physical-optimizer
  • Focused limit_pushdown regression tests

All local tests and checks pass.

Are there any user-facing changes?

Yes.

This fixes incorrect query results for order-sensitive LIMIT operations after physical-plan rewrites or repeated optimization.

Queries such as ORDER BY ... LIMIT will retain the required ordering information when limits are pushed down and recreated, preventing later optimization passes from treating the limit as order-insensitive.

@github-actions github-actions Bot added the optimizer Optimizer rules label Aug 10, 2026
@Himanshu-2005-code

Copy link
Copy Markdown
Author

Local validation completed for the change:

  • cargo check -p datafusion-physical-optimizer — passed
  • cargo clippy -p datafusion-physical-optimizer --all-targets -- -D warnings — passed
  • cargo fmt --all -- --check — passed
  • cargo test -p datafusion-physical-optimizer33 passed, 0 failed
  • Focused limit_pushdown tests — 2 passed, 0 failed

The implementation now preserves required_ordering when GlobalLimitExec / LocalLimitExec nodes are recreated during limit pushdown.

@github-actions github-actions Bot added the core Core DataFusion crate label Aug 10, 2026
@Himanshu-2005-code

Copy link
Copy Markdown
Author

I’ve finished the fix and regression coverage for the order-sensitive limit issue.

Summary:

preserved required_ordering when limits are rebuilt during limit pushdown
preserved the same metadata for fallback limits created by sort pushdown
added targeted regression tests for reinserted order-sensitive limits
Validation:

cargo check -p datafusion-physical-optimizer — passed
cargo clippy -p datafusion-physical-optimizer --all-targets -- -D warnings — passed
cargo fmt --all -- --check — passed
cargo test -p datafusion-physical-optimizer — passed
This addresses #24215 and is ready for review. Please let me know if you’d like any additional coverage or changes.

@neilconway

Copy link
Copy Markdown
Contributor

@Himanshu-2005-code Is there a GitHub issue for this bug? Please follow the PR template, and include more details about the user-visible behavior that is being changed and why. Thank you!

@github-actions

Copy link
Copy Markdown

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
     Cloning apache/main
    Building datafusion v54.1.0 (current)
       Built [ 105.338s] (current)
     Parsing datafusion v54.1.0 (current)
      Parsed [   0.035s] (current)
    Building datafusion v54.1.0 (baseline)
       Built [ 107.889s] (baseline)
     Parsing datafusion v54.1.0 (baseline)
      Parsed [   0.036s] (baseline)
    Checking datafusion v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.578s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 215.572s] datafusion
    Building datafusion-physical-optimizer v54.1.0 (current)
       Built [  42.348s] (current)
     Parsing datafusion-physical-optimizer v54.1.0 (current)
      Parsed [   0.022s] (current)
    Building datafusion-physical-optimizer v54.1.0 (baseline)
       Built [  40.849s] (baseline)
     Parsing datafusion-physical-optimizer v54.1.0 (baseline)
      Parsed [   0.022s] (baseline)
    Checking datafusion-physical-optimizer v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.115s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type GlobalRequirements is no longer UnwindSafe, in /home/runner/work/datafusion/datafusion/datafusion/physical-optimizer/src/limit_pushdown.rs:96
  type GlobalRequirements is no longer RefUnwindSafe, in /home/runner/work/datafusion/datafusion/datafusion/physical-optimizer/src/limit_pushdown.rs:96

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  84.980s] datafusion-physical-optimizer

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Aug 11, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.05128% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.01%. Comparing base (047f531) to head (7614aeb).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/physical-optimizer/src/pushdown_sort.rs 33.33% 6 Missing ⚠️
...atafusion/physical-optimizer/src/limit_pushdown.rs 96.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24231      +/-   ##
==========================================
- Coverage   81.01%   81.01%   -0.01%     
==========================================
  Files        1106     1106              
  Lines      384104   384129      +25     
  Branches   384104   384129      +25     
==========================================
+ Hits       311194   311209      +15     
- Misses      54566    54573       +7     
- Partials    18344    18347       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

auto detected api change Auto detected API change core Core DataFusion crate optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LimitPushdown and sort pushdown re-create limit nodes without required_ordering, losing order-sensitivity for later passes

3 participants