fix: preserve required ordering in limit pushdown - #24231
fix: preserve required ordering in limit pushdown#24231Himanshu-2005-code wants to merge 4 commits into
Conversation
|
Local validation completed for the change:
The implementation now preserves |
|
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 cargo check -p datafusion-physical-optimizer — passed |
|
@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! |
|
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 |
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
|
Thanks for pointing that out. I’ve updated the PR description to link #24215 and added the rationale, user-visible behavior, changes, and testing details. |
bvolpato
left a comment
bvolpato
left a comment
There was a problem hiding this comment.
Other than these inline comments, this looks good to me.
| let inner = if let Some(fetch) = sort_child.fetch() { | ||
| inner.with_fetch(Some(fetch)).unwrap_or_else(|| { | ||
| Arc::new(LocalLimitExec::new(inner, fetch)) | ||
| let mut limit = |
There was a problem hiding this comment.
Could this remain a LocalLimitExec while attaching required_ordering? In this SPM path, inner can still have multiple partitions, while GlobalLimitExec requires a single input partition and this rule runs after distribution enforcement. Would that make the resulting plan fail sanity checking or execution for an exact-pushdown source that does not support with_fetch()?
| skip: usize, | ||
| satisfied: bool, | ||
| preserve_order: bool, | ||
| required_ordering: Option<LexOrdering>, |
There was a problem hiding this comment.
GlobalRequirements is public, and replacing the bool with Option<LexOrdering> removes its UnwindSafe and RefUnwindSafe auto-trait implementations according to cargo-semver-checks. Is this compatibility break a concern here, or should ordering state be carried without changing those public auto traits?
|
Is it possible to find such query that shows ordering is loss but shouldn't and add it in sqllogictest? |
5 participants
Which issue does this PR close?
required_ordering, losing order-sensitivity for later passes #24215Rationale for this change
Queries using
ORDER BY ... LIMITcan 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 ... LIMITqueries to return different or incorrect rows.What changes are included in this PR?
required_orderingwhenGlobalLimitExecandLocalLimitExecnodes are recreated during limit pushdown.Are these changes tested?
Yes.
cargo check -p datafusion-physical-optimizercargo clippy -p datafusion-physical-optimizer --all-targets -- -D warningscargo fmt --all -- --checkcargo test -p datafusion-physical-optimizerlimit_pushdownregression testsAll local tests and checks pass.
Are there any user-facing changes?
Yes.
This fixes incorrect query results for order-sensitive
LIMIToperations after physical-plan rewrites or repeated optimization.Queries such as
ORDER BY ... LIMITwill retain the required ordering information when limits are pushed down and recreated, preventing later optimization passes from treating the limit as order-insensitive.