You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kita-renji (via GitHub)" <gi...@apache.org> on 2026/09/29 04:14:32 UTC
[PR] fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results [datafusion]
kita-renji opened a new pull request, #25860:
URL: https://github.com/apache/datafusion/pull/25860
## Which issue does this PR close?
- Closes #25859.
## Rationale for this change
Window aggregates over a `RANGE ... N PRECEDING` frame return results that depend on how the input is split into batches when the ORDER BY value is NULL. For a NULL row the frame is its group of NULL peers, and when that group spans batches the row is computed before the rest of the group arrives. At default settings a query over 20,000 rows with 10,000 NULL keys returns a count of 6384 instead of 10000 for 6,384 of the NULL rows. Details and repros are in the issue.
## What changes are included in this PR?
- `WindowFrame::new_bounds`: a `RANGE` frame is causal only when it ends at `UNBOUNDED PRECEDING`. With an offset end bound, the frame of a NULL or NaN row ends at its last peer, which can be a later row. `GROUPS` keeps the current rule, since NULL peers form an ordinary group there.
- `is_end_bound_safe_for_range`: a `PRECEDING` end bound is handled like `CURRENT ROW`, so the row waits until an input row past its peer group arrives (`is_row_ahead`) or the partition ends. An `N PRECEDING` frame only reaches the end of the buffer when the row's frame ends at its last peer, so the check only holds back NULL and NaN rows.
Rows with other ORDER BY values behave as before: their `N PRECEDING` frame ends before the current row, so it never reaches the end of the buffer and results are still produced as the input streams. RANGE frames planned from SQL were already non-causal (their bounds are strings until type coercion), so their plans don't change; the second change is what fixes them. The first change fixes frames built from typed bounds (reversed frames, the DataFrame API, proto), where built-in window functions such as `nth_value` were affected too.
## What is the testing strategy for this PR?
New cases at the end of `window.slt`, with `batch_size = 2` so the peers span batches: `sum`/`count`/`max` over `5 PRECEDING AND 1 PRECEDING` and `UNBOUNDED PRECEDING AND 1 PRECEDING`, NULLS FIRST and DESC orderings, a `FOLLOWING` frame that the planner reverses to reuse another window's sort (covering `nth_value`), and a group of NaN keys. Expected values match DuckDB 1.5.5 and Postgres 17, and all four queries fail without the fix.
I also ran a randomized check outside the test suite: random tables with NULL keys, random RANGE/GROUPS/ROWS frames and functions whose result doesn't depend on tie order, compared across batch sizes 1, 2, 3, 7 and 8192 and against DuckDB. Main had 31 batch-dependent results in the first 1,000 queries; this branch had none in 7,500, and none in variants with NaN keys and with an unbounded ordered source (Linear mode, PARTITION BY). The whole sqllogictest suite and the window fuzz tests (`--features extended_tests`) pass.
## Are there any user-facing changes?
Queries with NULL or NaN values in the ORDER BY column of a `RANGE ... N PRECEDING` window now return the same results regardless of batch size. `WindowFrame::is_causal()` now returns false for RANGE frames with an offset end bound. For frames built from typed bounds this can drop an ordering that a set-monotonic aggregate used to add to the window output, so a plan may need an extra sort; SQL-planned frames were already non-causal, and no sqllogictest plan changed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25860:
URL: https://github.com/apache/datafusion/pull/25860#issuecomment-5883729507
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25860?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.57%. Comparing base ([`bdad988`](https://app.codecov.io/gh/apache/datafusion/commit/bdad988f23f4f8a45a71409e7d7f24a326ca909c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`75e325f`](https://app.codecov.io/gh/apache/datafusion/commit/75e325fa7681a2709282d21e6fdfb604dcc89c93?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25860 +/- ##
=======================================
Coverage 82.57% 82.57%
=======================================
Files 1142 1142
Lines 440861 440859 -2
Branches 440861 440859 -2
=======================================
+ Hits 364047 364051 +4
+ Misses 54818 54809 -9
- Partials 21996 21999 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25860?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results [datafusion]
Posted by "Nagato-Yuzuru (via GitHub)" <gi...@apache.org>.
Nagato-Yuzuru commented on PR #25860:
URL: https://github.com/apache/datafusion/pull/25860#issuecomment-6200027926
Hi @kita-renji, thank you for this fix.
I work on the fuzz test plan in #24243. When I added NULL values to the window fuzz test, the test also found #25859. I opened the fuzz change as #26386.
On my fuzz branch, your patch fixes the error and without it the test fails. I tested all four sort orders (ASC/DESC, NULLS FIRST/LAST).
If you agree, you can cherry-pick the commits of #26386 into this PR. It make the fix and its fuzz test are in one PR. The commits apply to this branch without conflicts:
```shell
git fetch https://github.com/Nagato-Yuzuru/datafusion.git fuzz-window-nulls
git cherry-pick FETCH_HEAD~2..FETCH_HEAD
```
If you prefer to keep them separate also good. #26386 can merge after this PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #25860:
URL: https://github.com/apache/datafusion/pull/25860
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org