You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ump45nose (via GitHub)" <gi...@apache.org> on 2026/10/23 13:14:58 UTC
[PR] fix: saturate window frame lookahead instead of overflowing on huge FOLLOWING bounds [datafusion]
ump45nose opened a new pull request, #26662:
URL: https://github.com/apache/datafusion/pull/26662
## Which issue does this PR close?
- Closes #26658.
## Rationale for this change
A `ROWS` window frame with a huge `FOLLOWING` bound (for example `18446744073709551615 FOLLOWING`) combined with `ORDER BY ... LIMIT` panics with `attempt to add with overflow` in debug builds. In release builds the addition wraps, so `LimitPushPastWindows` could give the `SortExec` below the window a fetch that is far too small and drop rows the window needs.
After fixing the optimizer rule, the same query panicked a second time in `WindowFrameContext::calculate_range_rows` (`window_state.rs`), which computes the frame end as `idx + n + 1` with the same unchecked bound. So both places need fixing for the query in the issue to return its result.
## What changes are included in this PR?
- `limit_pushdown_past_window.rs`: use `saturating_add` when growing the limit by the window lookahead (`fetch + lookahead`, `lookahead + max_rel`) and when adding `skip` to a `GlobalLimitExec` fetch. A saturated fetch is still correct, it just means the sort keeps all rows. This matches how `input_fetch` in `sort_pushdown.rs` already handles the same kind of sum.
- `window_state.rs`: use `saturating_add` for the `FOLLOWING n` start and end of a `ROWS` frame. The result is already clamped to the partition length, so saturating gives the right range.
This touches the same lines in `limit_pushdown_past_window.rs` as the refactor in #26660, which leaves these sums to #26658. The change here is small, so whichever lands second should be easy to rebase.
## What is the testing strategy for this PR?
- New cases in `window_limits.slt`: the query from the issue (`sum(x) OVER (... ROWS BETWEEN CURRENT ROW AND 18446744073709551615 FOLLOWING) ... LIMIT 2`), and a `lead(x, 1)` variant that adds its own lookahead on top of the huge frame bound. Without this change the first case panics at `limit_pushdown_past_window.rs:198`; with only the optimizer fix it panics at `window_state.rs:287`.
- Locally: all `window*.slt` sqllogictests, `cargo test -p datafusion-expr --lib window_state`, `cargo test -p datafusion-physical-optimizer --lib limit_pushdown_past_window`, `cargo fmt --all` and `cargo clippy -p datafusion-expr -p datafusion-physical-optimizer --all-targets --all-features -- -D warnings` pass.
## Are there any user-facing changes?
No API changes. Queries with very large `ROWS ... FOLLOWING` bounds now return results instead of panicking (or, in release builds, possibly returning too few rows).
---
This PR was prepared with AI assistance. I reviewed the change and ran the tests listed above.
--
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: saturate window frame lookahead instead of overflowing on huge FOLLOWING bounds [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #26662:
URL: https://github.com/apache/datafusion/pull/26662#discussion_r4369538964
##########
datafusion/physical-optimizer/src/limit_pushdown_past_window.rs:
##########
@@ -194,16 +197,21 @@ fn apply_limit(
ctx.lookahead = 0;
return Some(Transformed::no(Arc::clone(node)));
};
+ let fetch = fetch.saturating_add(ctx.lookahead);
Review Comment:
When this sum saturates, the sort below becomes `SortExec: TopK(fetch=18446744073709551615)`. TopK keeps every row in memory and can't spill. With 2M rows and a 24MB memory limit, the issue query fails with `Resources exhausted ... TopK[0]`. The same query with `enable_window_limits = false` returns the right rows, because the full sort spills. A fetch that overflows can't drop any rows, so could we skip the pushdown instead of saturating?
```sql
set datafusion.execution.target_partitions = 1;
CREATE TABLE big AS SELECT (value * 7919) % 1000003 AS a, value AS x FROM generate_series(1, 2000000);
set datafusion.runtime.memory_limit = '24M';
set datafusion.execution.sort_spill_reservation_bytes = 1048576;
SELECT a, sum(x) OVER (ORDER BY a ROWS BETWEEN CURRENT ROW AND 18446744073709551615 FOLLOWING) s
FROM big ORDER BY a LIMIT 2;
```
```rs
let Some(fetch) = fetch.checked_add(ctx.lookahead) else {
ctx.lookahead = 0;
return Some(Transformed::no(Arc::clone(node)));
};
let fetch = match node.fetch() {
None => fetch,
Some(existing) => cmp::min(existing, fetch),
};
```
With this change the query succeeds, the plan shows a plain `SortExec`, and `window_limits.slt` still passes.
##########
datafusion/sqllogictest/test_files/window_limits.slt:
##########
@@ -862,3 +862,26 @@ physical_plan
03)----BoundedWindowAggExec: wdw=[lead(employees.salary,Int64(2)) ORDER BY [employees.empno ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW: Field { "lead(employees.salary,Int64(2)) ORDER BY [employees.empno ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW": nullable UInt64 }, frame: ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
04)------SortExec: TopK(fetch=5), expr=[empno@0 ASC NULLS LAST], preserve_partitioning=[false]
05)--------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100_with_dates.csv]]}, projection=[empno, salary], file_type=csv, has_header=true
+
+# A huge FOLLOWING bound must not overflow when the limit is grown by the
Review Comment:
These two cases cover the frame end and the `grow_limit` sum, but nothing reaches the `Following` start bound in `calculate_range_rows`. On main that line overflows for any row after the first. Could you add this case? It returns the rows below with your change.
```sql
query II
SELECT a, sum(x) OVER (ORDER BY a ROWS BETWEEN 18446744073709551615 FOLLOWING AND 18446744073709551615 FOLLOWING) s
FROM huge_frame_t ORDER BY a LIMIT 2;
----
1 NULL
2 NULL
```
--
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: saturate window frame lookahead instead of overflowing on huge FOLLOWING bounds [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #26662:
URL: https://github.com/apache/datafusion/pull/26662
--
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: saturate window frame lookahead instead of overflowing on huge FOLLOWING bounds [datafusion]
Posted by "ump45nose (via GitHub)" <gi...@apache.org>.
ump45nose commented on PR #26662:
URL: https://github.com/apache/datafusion/pull/26662#issuecomment-6407239437
Thanks for the review @jayzhan211, and for the repro. You're right, a saturated fetch just turns into an unbounded TopK. I switched to `checked_add` so the pushdown is skipped when the fetch would overflow, and added the `FOLLOWING`-start case to `window_limits.slt`. Pushed in 43debca.
--
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: saturate window frame lookahead instead of overflowing on huge FOLLOWING bounds [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #26662:
URL: https://github.com/apache/datafusion/pull/26662#issuecomment-6407388920
Thanks @ump45nose !
--
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