fix: sort pushed below GlobalLimitExec ignores skip, returning too few rows - #24958
fix: sort pushed below GlobalLimitExec ignores skip, returning too few rows#24958jayzhan211 wants to merge 1 commit into
GlobalLimitExec ignores skip, returning too few rows#24958Conversation
Sort pushdown seeds the `fetch` it carries from the limit's own `fetch()`,
ignoring `skip`. A sort pushed below `GlobalLimitExec: skip=5, fetch=10`
therefore became `TopK(fetch=10)`, and the limit then skipped 5 of those 10
rows:
SELECT * FROM (SELECT * FROM t LIMIT 10 OFFSET 5) ORDER BY a
returned 5 rows instead of 10. The same seed is used when a parent's ordering
requirement is already satisfied by a `GlobalLimitExec` child.
Use `skip + fetch` for `GlobalLimitExec` in both places.
99b16e7 to
0c73442
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24958 +/- ##
==========================================
- Coverage 81.62% 81.62% -0.01%
==========================================
Files 1124 1124
Lines 412462 412469 +7
Branches 412462 412469 +7
==========================================
- Hits 336684 336678 -6
- Misses 55965 55973 +8
- Partials 19813 19818 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ryux1
left a comment
There was a problem hiding this comment.
The ordinary OFFSET case is covered well. I found one boundary condition in the new fetch calculation.
| let skip = plan | ||
| .downcast_ref::<GlobalLimitExec>() | ||
| .map_or(0, |limit| limit.skip()); | ||
| Some(fetch + skip) |
There was a problem hiding this comment.
GlobalLimitExec::new accepts arbitrary usize values, so this addition can overflow when skip + fetch > usize::MAX: debug builds panic, while optimized builds wrap and may turn the pushed sort into e.g. TopK(fetch=0), producing incorrect results. DataFusion's combine_limit uses saturating_add for the analogous limit composition. Could this use fetch.saturating_add(skip) as well, with a unit test covering skip = usize::MAX, fetch = 1 (or an equivalent overflow boundary)?
Which issue does this PR close?
Rationale for this change
ORDER BYapplied on top of aLIMIT ... OFFSETsubquery returns too few rows.EnforceSortingpushes the sort below theGlobalLimitExecand converts it into aTopK. The TopK's fetch was seeded from
GlobalLimitExec::fetch(), which is thelimit's output row count and ignores
skip. So the TopK kept only 4 rows, thelimit then skipped 3 of them, and the query returned a single row instead of 4.
The same seeding happens in two places in
sort_pushdown.rs: when a root's directchild is a
GlobalLimitExec(assign_initial_requirements) and when the pushdowndescends through one (
pushdown_sorts_helper). Both had the bug.What changes are included in this PR?
input_fetchhelper insort_pushdown.rsthat returnsskip + fetchforGlobalLimitExecand plainfetch()for every other operator.the TopK below a
GlobalLimitExecretains enough rows for the limit to skip andstill return
fetchrows.What is the testing strategy for this PR?
limit.slt: newEXPLAIN+ result test forLIMIT 4 OFFSET 3underORDER BY,showing
TopK(fetch=7)belowGlobalLimitExec: skip=3, fetch=4and the correct4 rows.
ensure_requirements.rs: two unit tests, one per code path(
test_sort_pushed_below_limit_with_skip_keeps_skip_rowsandtest_parent_ordering_over_limit_with_skip_keeps_skip_rows), asserting thepushed sort has
fetch=15forskip=5, fetch=10.Are there any user-facing changes?
Queries with a sort above
LIMIT ... OFFSETnow return the correct number of rows.No API changes.