Repository navigation
fix: wait for the whole NULL peer group before emitting RANGE N PRECEDING window results - #25860
Merged
Merged
Conversation
…window results For a RANGE frame with an offset end bound, the frame of a row whose ORDER BY value is NULL (or NaN) ends at its last peer, which can arrive in a later batch. BoundedWindowAggExec emitted such rows early, so results depended on batch boundaries. - WindowFrame::new_bounds: RANGE frames are causal only when they end at UNBOUNDED PRECEDING. - is_end_bound_safe_for_range: handle a PRECEDING end bound like CURRENT ROW and wait until a row past the peer group arrives. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
github-actions
Bot
added
logical-expr
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
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: git fetch https://github.com/Nagato-Yuzuru/datafusion.git fuzz-window-nulls
git cherry-pick FETCH_HEAD~2..FETCH_HEADIf you prefer to keep them separate also good. #26386 can merge after this PR. |
jayzhan211
approved these changes
Oct 18, 2026
jayzhan211
left a comment
jayzhan211
left a comment
Contributor
There was a problem hiding this comment.
Thanks @kita-renji 👍🏻
charan-rathore
pushed a commit
to charan-rathore/datafusion
that referenced
this pull request
Oct 19, 2026
<!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> Part of apache#24243. This PR adds NULL values to the window fuzz test, as planned. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> The window fuzz test had two gaps: - The test data had no NULL values. - The test used only one sort order: `ASC NULLS FIRST`. With these gaps closed, the test finds wrong results on `main`. All of them come from one known error: apache#25859. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> - Add NULL values to the test data. The NULL rate changes between 0, 0.01, 0.1 and 0.5. - Give each sort column a random sort direction and a random NULL position. Note: a seed now gives different data than before this PR. ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> ```bash cargo test --profile ci -p datafusion --features extended_tests --test fuzz -- window_fuzz ``` On `main`, the test fails. For example: ``` WindowFrame { units: Range, start_bound: Preceding(Int32(6)), end_bound: Preceding(Int32(5)), .. }, window_fn: max(x), random_seed: 267, null_pct: 0.5, pb_cols: ["a"], .. ``` All failures have a `RANGE` frame that ends in `N PRECEDING`, and NULL values in the data. They occur with all four sort orders (ASC/DESC, NULLS FIRST/LAST). ## Additional notes apache#25860 fixes apache#25859. With apache#25860, this test passes. This PR must merge after apache#25860. Or, if the author of apache#25860 agrees, these commits can go into apache#25860. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. --> No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
RANGE ... N PRECEDINGdepend on batch boundaries when the ORDER BY value is NULL #25859.Rationale for this change
Window aggregates over a
RANGE ... N PRECEDINGframe 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: aRANGEframe is causal only when it ends atUNBOUNDED 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.GROUPSkeeps the current rule, since NULL peers form an ordinary group there.is_end_bound_safe_for_range: aPRECEDINGend bound is handled likeCURRENT ROW, so the row waits until an input row past its peer group arrives (is_row_ahead) or the partition ends. AnN PRECEDINGframe 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 PRECEDINGframe 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 asnth_valuewere affected too.What is the testing strategy for this PR?
New cases at the end of
window.slt, withbatch_size = 2so the peers span batches:sum/count/maxover5 PRECEDING AND 1 PRECEDINGandUNBOUNDED PRECEDING AND 1 PRECEDING, NULLS FIRST and DESC orderings, aFOLLOWINGframe that the planner reverses to reuse another window's sort (coveringnth_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 PRECEDINGwindow 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