geoffreyclaude opened a new pull request, #24932:
URL: https://github.com/apache/datafusion/pull/24932
## Which issue does this PR close?
- Closes #24927.
## Rationale for this change
With multiple partitions, `SELECT DISTINCT ... ORDER BY ... LIMIT` can return
fewer rows than requested even when enough distinct values exist.
`DISTINCT` runs in partial and final stages. Sort pushdown was moving the
final
TopK `fetch` (the sort's row limit) below the final aggregation, while
duplicate
partial results still existed:
```text
SortPreservingMergeExec: fetch=3
AggregateExec: final DISTINCT
SortExec: TopK(fetch=3) <-- limit is applied too early
AggregateExec: partial DISTINCT
```
Duplicates could therefore fill the limit and discard another value before
the
final `DISTINCT` had a chance to remove them.
I bisected this regression to
[`450c861a8c`](https://github.com/apache/datafusion/commit/450c861a8c38f8934b134368949606aaf3287792)
(#14821), first released in 47.0.0. That refactor made the
ordering-satisfied fast path forward `fetch` to the current plan's children.
It correctly established that the final aggregate preserved the requested
ordering, but ordering preservation does not imply that moving a limit is
safe.
The shortcut consequently bypassed the existing limit-pushdown and
cardinality
checks. The direct parent returns all three values; this commit returns too
few.
## What changes are included in this PR?
The ordering-satisfied fast path is now used only when there is no pending
fetch, or when the current leaf or `SortPreservingMergeExec` can retain the
fetch itself. Other fetched non-leaf plans use the existing guarded pushdown
path instead.
For `AggregateExec`, those existing checks reject the unsafe pushdown,
keeping
TopK above the final `DISTINCT`. Ordering-only pushdown and the existing leaf
and sort-preserving-merge optimizations remain unchanged.
## What is the testing strategy for this PR?
An SQLLogicTest in `limit.slt` uses the exact two-partition query from #24927
and verifies that it returns `0`, `1`, and `2`. The case reproduces the
missing
`1` without the fix.
I also ran:
- `cargo fmt --all`
- `cargo clippy --all-targets --all-features -- -D warnings`
- `cargo test --test sqllogictests -- limit.slt`
- `cargo test -p datafusion --test core_integration
physical_optimizer::enforce_sorting::`
The contributor-guide extended workspace run had one environment-only
failure:
`test_sort_10k_mem` exceeded the host's 1,024-open-file limit. It passed in
isolation after raising that limit to 8,192.
## Are there any user-facing changes?
Yes. Affected queries now return all requested distinct rows. There are no
API
or configuration changes.
This PR was developed with AI assistance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
