You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/10/18 09:09:45 UTC
(datafusion) branch gh-readonly-queue/main/pr-25860-5b5edc7f3cbc46106455e78d2a10551ed57e884d created (now 3470bd0fbc)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-25860-5b5edc7f3cbc46106455e78d2a10551ed57e884d
in repository https://gitbox.apache.org/repos/asf/datafusion.git
at 3470bd0fbc fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results (#25860)
This branch includes the following new commits:
new 3470bd0fbc fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results (#25860)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion) 01/01: fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results (#25860)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-25860-5b5edc7f3cbc46106455e78d2a10551ed57e884d
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 3470bd0fbc016df5280493bb41c2e7ced3a4e626
Author: Kita Renji <73...@users.noreply.github.com>
AuthorDate: Sun Oct 18 09:09:20 2026 +0000
fix: wait for the whole NULL peer group before emitting RANGE `N PRECEDING` window results (#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)
Co-authored-by: Claude Opus 5.5 (1M context) <no...@anthropic.com>
---
datafusion/expr/src/window_frame.rs | 8 ++-
datafusion/physical-expr/src/window/window_expr.rs | 13 ++--
datafusion/sqllogictest/test_files/window.slt | 77 ++++++++++++++++++++++
3 files changed, 88 insertions(+), 10 deletions(-)
diff --git a/datafusion/expr/src/window_frame.rs b/datafusion/expr/src/window_frame.rs
index 018ae8216d..68a722eaf6 100644
--- a/datafusion/expr/src/window_frame.rs
+++ b/datafusion/expr/src/window_frame.rs
@@ -224,7 +224,13 @@ impl WindowFrame {
}
_ => true,
},
- WindowFrameUnits::Range | WindowFrameUnits::Groups => match &end_bound {
+ // For a row whose ORDER BY value is NULL or NaN, an offset bound
+ // ends the RANGE frame at its last peer, which may be a later row.
+ WindowFrameUnits::Range => matches!(
+ &end_bound,
+ WindowFrameBound::Preceding(value) if value.is_null()
+ ),
+ WindowFrameUnits::Groups => match &end_bound {
WindowFrameBound::Preceding(value) => {
if value.is_null() {
// Unbounded preceding
diff --git a/datafusion/physical-expr/src/window/window_expr.rs b/datafusion/physical-expr/src/window/window_expr.rs
index 690731f4a9..5f6bbc46c2 100644
--- a/datafusion/physical-expr/src/window/window_expr.rs
+++ b/datafusion/physical-expr/src/window/window_expr.rs
@@ -507,15 +507,10 @@ fn is_end_bound_safe_for_range(
idx: usize,
) -> Result<bool> {
match end_bound {
- WindowFrameBound::Preceding(value) => {
- let zero = ScalarValue::new_zero(&value.data_type())?;
- if value.eq(&zero) {
- is_row_ahead(orderby_col, most_recent_ob_col, sort_options)
- } else {
- Ok(true)
- }
- }
- WindowFrameBound::CurrentRow => {
+ // A frame ending in `N PRECEDING` can only reach the end of the buffer
+ // when the current row's frame ends at its last peer, as for a NULL or
+ // NaN ORDER BY value, so it is safe once a row past its peers arrives.
+ WindowFrameBound::Preceding(_) | WindowFrameBound::CurrentRow => {
is_row_ahead(orderby_col, most_recent_ob_col, sort_options)
}
WindowFrameBound::Following(delta) => {
diff --git a/datafusion/sqllogictest/test_files/window.slt b/datafusion/sqllogictest/test_files/window.slt
index 254fdf20e5..7c92514e72 100644
--- a/datafusion/sqllogictest/test_files/window.slt
+++ b/datafusion/sqllogictest/test_files/window.slt
@@ -7345,3 +7345,80 @@ SELECT a FROM window_in_filter_t LIMIT count(*);
statement ok
DROP TABLE window_in_filter_t;
+
+# RANGE frames ending in `N PRECEDING`: for a row whose ORDER BY value is NULL
+# the frame is its NULL peer group, so the result must not depend on how the
+# NULL peers are split across input batches.
+statement ok
+CREATE TABLE range_null_peers(id INT, x INT, v INT) AS VALUES
+ (1, 1, 10), (2, 2, 20), (3, NULL, 30), (4, NULL, 40), (5, NULL, 50);
+
+statement ok
+set datafusion.execution.batch_size = 2;
+
+query IIIII
+SELECT id,
+ sum(v) OVER (ORDER BY x RANGE BETWEEN 5 PRECEDING AND 1 PRECEDING),
+ count(*) OVER (ORDER BY x RANGE BETWEEN 5 PRECEDING AND 1 PRECEDING),
+ max(v) OVER (ORDER BY x RANGE BETWEEN 5 PRECEDING AND 1 PRECEDING),
+ sum(v) OVER (ORDER BY x RANGE BETWEEN UNBOUNDED PRECEDING AND 1 PRECEDING)
+FROM range_null_peers ORDER BY id;
+----
+1 NULL 0 NULL NULL
+2 10 1 10 10
+3 120 3 50 150
+4 120 3 50 150
+5 120 3 50 150
+
+query III
+SELECT id,
+ sum(v) OVER (ORDER BY x NULLS FIRST RANGE BETWEEN 5 PRECEDING AND 1 PRECEDING),
+ sum(v) OVER (ORDER BY x DESC RANGE BETWEEN 5 PRECEDING AND 1 PRECEDING)
+FROM range_null_peers ORDER BY id;
+----
+1 NULL 20
+2 10 NULL
+3 120 120
+4 120 120
+5 120 120
+
+# The FOLLOWING frames are reversed to `5 PRECEDING AND 1 PRECEDING` so they can
+# reuse the sort of the first window
+query IIII
+SELECT id,
+ count(*) OVER (ORDER BY x DESC NULLS LAST, id),
+ sum(v) OVER (ORDER BY x ASC NULLS FIRST RANGE BETWEEN 1 FOLLOWING AND 5 FOLLOWING),
+ nth_value(1, 3) OVER (ORDER BY x ASC NULLS FIRST RANGE BETWEEN 1 FOLLOWING AND 5 FOLLOWING)
+FROM range_null_peers ORDER BY id;
+----
+1 2 20 NULL
+2 1 NULL NULL
+3 3 120 1
+4 4 120 1
+5 5 120 1
+
+# NaN sorts as a single peer group too, and `NaN - 1` is NaN
+statement ok
+CREATE TABLE range_nan_peers(id INT, x DOUBLE) AS VALUES
+ (1, 1.0), (2, 'NaN'), (3, 'NaN'), (4, 'NaN'), (5, 2.0);
+
+query III
+SELECT id,
+ count(*) OVER (ORDER BY x RANGE BETWEEN UNBOUNDED PRECEDING AND 1 PRECEDING),
+ count(*) OVER (ORDER BY x DESC RANGE BETWEEN UNBOUNDED PRECEDING AND 1 PRECEDING)
+FROM range_nan_peers ORDER BY id;
+----
+1 0 4
+2 5 3
+3 5 3
+4 5 3
+5 1 3
+
+statement ok
+reset datafusion.execution.batch_size;
+
+statement ok
+DROP TABLE range_null_peers;
+
+statement ok
+DROP TABLE range_nan_peers;
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org