Skip to content

fix: skip WindowTopN rewrite when fetch is 0 to avoid PartitionedTopK panic - #24405

Merged
viirya merged 1 commit into
apache:mainfrom
viirya:fix-window-topn-k0-panic
Aug 16, 2026
Merged

fix: skip WindowTopN rewrite when fetch is 0 to avoid PartitionedTopK panic#24405
viirya merged 1 commit into
apache:mainfrom
viirya:fix-window-topn-k0-panic

Conversation

@viirya

@viirya viirya commented Aug 15, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

With datafusion.optimizer.enable_window_topn = true, a query filtering a partitioned ROW_NUMBER()/RANK() with rn < 1 (or the flipped 1 > rn) panics with PartitionedTopK requires k > 0.

ROW_NUMBER/RANK are always >= 1, so such a predicate matches no rows and the correct result is empty — it must not panic. The WindowTopN rule's extract_window_limit maps rn < K to a fetch of K - 1; for K = 1 that fetch is 0, which is passed to PartitionedTopKExec::try_new, whose assert!(k > 0) panics. With the optimization disabled the same query correctly returns no rows.

What changes are included in this PR?

  • In WindowTopN::try_transform, bail out of the rewrite when the computed limit is 0, letting the regular FilterExec produce the (empty) result.
  • Add regression cases to window_topn.slt for rn < 1, 1 > rn, and rn <= 0 (all must return empty without panicking).

Are these changes tested?

Yes — the new sqllogictest cases fail (panic) without the fix and pass with it. Existing window_topn tests continue to pass.

Are there any user-facing changes?

No API changes. rn < 1 / 1 > rn now returns an empty result instead of panicking when enable_window_topn is on, matching the behavior when it is off.

@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Aug 15, 2026
… panic

A predicate like `rn < 1` (or the flipped `1 > rn`) makes the WindowTopN
rule compute a fetch of 0. Since ROW_NUMBER/RANK are always >= 1, no row can
satisfy such a predicate and the correct result is empty. The rule passed
fetch = 0 to `PartitionedTopKExec::try_new`, whose `assert!(k > 0)` panicked.

Bail out of the rewrite when the computed limit is 0 and let the regular
FilterExec produce the (empty) result instead.

Closes apache#24404

Co-authored-by: Claude Code
@viirya
viirya force-pushed the fix-window-topn-k0-panic branch from 72de4dc to 0ac4c9c Compare August 16, 2026 00:04
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.18%. Comparing base (e7e037d) to head (0ac4c9c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24405      +/-   ##
==========================================
- Coverage   81.18%   81.18%   -0.01%     
==========================================
  Files        1110     1110              
  Lines      388906   388909       +3     
  Branches   388906   388909       +3     
==========================================
  Hits       315733   315733              
- Misses      54576    54578       +2     
- Partials    18597    18598       +1     

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

@viirya
viirya added this pull request to the merge queue Aug 16, 2026
@viirya

viirya commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Thanks @jayzhan211 for review!

Merged via the queue into apache:main with commit b551ba0 Aug 16, 2026
40 checks passed
@viirya
viirya deleted the fix-window-topn-k0-panic branch August 16, 2026 07:14
imtherealnaska pushed a commit to imtherealnaska/datafusion that referenced this pull request Aug 16, 2026
… panic (apache#24405)

## Which issue does this PR close?

- Closes apache#24404.

## Rationale for this change

With `datafusion.optimizer.enable_window_topn = true`, a query filtering
a partitioned `ROW_NUMBER()`/`RANK()` with `rn < 1` (or the flipped `1 >
rn`) panics with `PartitionedTopK requires k > 0`.

`ROW_NUMBER`/`RANK` are always `>= 1`, so such a predicate matches no
rows and the correct result is empty — it must not panic. The
`WindowTopN` rule's `extract_window_limit` maps `rn < K` to a fetch of
`K - 1`; for `K = 1` that fetch is `0`, which is passed to
`PartitionedTopKExec::try_new`, whose `assert!(k > 0)` panics. With the
optimization disabled the same query correctly returns no rows.

## What changes are included in this PR?

- In `WindowTopN::try_transform`, bail out of the rewrite when the
computed limit is `0`, letting the regular `FilterExec` produce the
(empty) result.
- Add regression cases to `window_topn.slt` for `rn < 1`, `1 > rn`, and
`rn <= 0` (all must return empty without panicking).

## Are these changes tested?

Yes — the new sqllogictest cases fail (panic) without the fix and pass
with it. Existing `window_topn` tests continue to pass.

## Are there any user-facing changes?

No API changes. `rn < 1` / `1 > rn` now returns an empty result instead
of panicking when `enable_window_topn` is on, matching the behavior when
it is off.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enable_window_topn panics on WHERE rn < 1 (PartitionedTopK requires k > 0)

3 participants