Repository navigation
Conversation
…he#26431) A SortExec with fetch = Some(0) over unsorted input built a TopK with k = 0, which panicked on assert!(k > 0) in TopKHeap::new. This is reachable when eliminate_limit does not run, e.g. with datafusion.optimizer.max_passes = 0. Treat a k = 0 TopK as finished from the start: it ignores input, emits no rows and still marks its dynamic filter complete. SortExec now checks finished before polling input, so it does not read any input in that case.
rgbuilds
left a comment
There was a problem hiding this comment.
Thanks for fixing this.
I traced the zero-k lifecycle through SortExec and TopK and verified the focused tests locally. The old implementation reproduces the panic, while the current implementation returns an empty result, completes the dynamic filter, and avoids polling the input.
The implementation looks sound to me. I left one non-blocking test suggestion.
| // Check `finished` before polling so that a TopK that | ||
| // is already complete (e.g. `fetch = 0`) does not read | ||
| // any input. | ||
| while !topk.finished { |
There was a problem hiding this comment.
Non-blocking test suggestion: could we add a focused SortExec::with_fetch(Some(0)) execution test using PanicExec or another first-poll-failing stream?
The unit test covers direct TopK construction, insertion, and emission, while the SLT covers the plan and empty result, but neither would detect a future change that polls the input once before checking finished. I verified locally that such a PanicExec test passes with this implementation.
Which issue does this PR close?
ORDER BY ... LIMIT 0panics in TopK (assertion failed: k > 0) wheneliminate_limitdoes not run #26431.Rationale for this change
ORDER BY ... LIMIT 0panics withassertion failed: k > 0when the plan reachesSortExec: TopK(fetch=0). The default optimizer hides this becauseeliminate_limitreplacesLIMIT 0with an empty relation, but it is reachable withdatafusion.optimizer.max_passes = 0, with custom optimizer rules, or by buildingSortExec::with_fetch(Some(0))directly. ALIMIT 0query should return no rows, not panic.What changes are included in this PR?
TopKHeap::newno longer assertsk > 0.TopK::try_newmarks ak = 0TopK asfinishedup front, andTopK::insert_batchreturns early whenk = 0(no row can enter the heap).SortExeccheckstopk.finishedbefore polling the next input batch, so afetch = 0TopK does not read its input. The loop behaves the same as before fork > 0.The fix is in
TopKitself, not a separate empty-stream branch inSortExec. This wayemit()still runs and marks the shared TopK dynamic filter as emitted/complete, as it does for any other TopK.What is the testing strategy for this PR?
topk::tests::test_topk_with_zero_k: ak = 0TopK is finished at construction, ignores an inserted batch, emits no rows, and marks its dynamic filter complete. Without this change it panics inTopKHeap::new.topk.sltcase that setsdatafusion.optimizer.max_passes = 0, checks that the plan isSortExec: TopK(fetch=0), and checks thatselect * from topk order by x limit 0returns no rows.cargo test -p datafusion-physical-plan --lib -- topk sorts::, thetopk.slt,limit*.sltandorder.sltsqllogictests,cargo fmt --all -- --checkandcargo clippy -p datafusion-physical-plan --all-targets --all-features -- -D warningsall pass. The newtopk.sltcase panics withassertion failed: k > 0without the fix.Are there any user-facing changes?
No API changes.
LIMIT 0with a TopK sort now returns an empty result instead of panicking.This PR was prepared with AI assistance. I reviewed the change and ran the tests listed above.