Skip to content

fix: preserve scan order during sort-preserving merge limit pushdown - #24273

Open
Theodus wants to merge 3 commits into
apache:mainfrom
edgeandnode:theodus/limit-pushdown-preserve-spm-order
Open

fix: preserve scan order during sort-preserving merge limit pushdown#24273
Theodus wants to merge 3 commits into
apache:mainfrom
edgeandnode:theodus/limit-pushdown-preserve-spm-order

Conversation

@Theodus

@Theodus Theodus commented Aug 11, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

Queries that filter ordered data and apply ORDER BY ... LIMIT can return incorrect rows when the physical plan contains a fetched SortPreservingMergeExec.

When the fetch is pushed into an ordered Parquet scan, LimitPushdown currently treats it as order-insensitive. This allows Parquet's limit-based row-group pruning to discard an earlier partially matched row group in favor of a later fully matched row group. The scan can therefore return later rows instead of the first rows in the requested ordering.

For example, a query shaped like:

SELECT key
FROM table
WHERE key >= 1
ORDER BY key ASC
LIMIT 5;

may skip the row group containing the lowest matching values and return values from a later row group.

What changes are included in this PR?

  • Treat a fetched SortPreservingMergeExec as order-sensitive during limit pushdown.
  • Propagate preserve_order = true when pushing its fetch into the underlying scan.
  • Add a regression test verifying that an ordered Parquet scan receives both limit = Some(5) and preserve_order = true.

This prevents order-insensitive Parquet limit pruning from changing which rows are eligible for an ordered limit.

Are these changes tested?

Yes. The new preserves_order_when_pushing_fetch_from_sort_preserving_merge regression test fails without this change because limit pushdown resets the scan's preserve_order flag to false.

Are there any user-facing changes?

Yes. Ordered queries with a pushed-down limit now return the earliest matching rows instead of rows from a later Parquet row group.

There are no public API changes.

@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate labels Aug 11, 2026

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

Cound we add an SQLTest E2E test? There is already similar examples in limit_pruning.slt.

@Theodus
Theodus force-pushed the theodus/limit-pushdown-preserve-spm-order branch from 36adcf3 to 2003a43 Compare August 11, 2026 20:02
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 11, 2026
@Theodus

Theodus commented Aug 11, 2026

Copy link
Copy Markdown
Author

@Nagato-Yuzuru thanks for the suggestion, added in 0dab6cf.

Signed-off-by: Theo Butler <theodusbutler@gmail.com>
Signed-off-by: Theo Butler <theodusbutler@gmail.com>
Signed-off-by: Theo Butler <theodusbutler@gmail.com>
@Theodus
Theodus force-pushed the theodus/limit-pushdown-preserve-spm-order branch from 2003a43 to e972cd3 Compare August 11, 2026 20:04
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.26%. Comparing base (e6be9cd) to head (e972cd3).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24273      +/-   ##
==========================================
+ Coverage   81.03%   81.26%   +0.22%     
==========================================
  Files        1107     1110       +3     
  Lines      385158   384908     -250     
  Branches   385158   384908     -250     
==========================================
+ Hits       312132   312796     +664     
+ Misses      54606    53642     -964     
- Partials    18420    18470      +50     

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

@Theodus
Theodus requested a review from Nagato-Yuzuru August 12, 2026 13:42

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

Other than this inline question, this looks good to me. Fix and regression coverage make sense.

global_state.skip = skip;
global_state.fetch = fetch;
global_state.preserve_order = limit_info.preserve_order;
global_state.preserve_order |= limit_info.preserve_order;

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.

Could we clear preserve_order when ordered fetch scope ends instead of carrying it into later limits? For example, with SortPreservingMergeExec(fetch=5) -> SortExec -> LocalLimitExec(fetch=10) -> DataSourceExec, SortExec clears outer fetch, but this |= keeps flag set and applies it to independent inner limit. That disables Parquet limit pruning and file-stream work stealing even though inner limit is not order-sensitive. Is this a concern, or am I missing where state is reset?

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

Labels

core Core DataFusion crate optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LimitPushdown can return incorrect rows for fetched SortPreservingMergeExec over ordered Parquet scans

4 participants