[I] `LimitPushPastWindows` panics with "attempt to add with overflow" for a large `ROWS ... FOLLOWING` bound [datafusion]

["jayzhan211 (via GitHub)" <gi...@apache.org>]

jayzhan211 opened a new issue, #26658:
URL: https://github.com/apache/datafusion/issues/26658

   ### Describe the bug
   
   A window frame with a very large `FOLLOWING` bound makes the physical optimizer panic in debug builds:
   
   ```sql
   CREATE TABLE t(a INT, x INT) AS VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5);
   
   SELECT a, sum(x) OVER (ORDER BY a ROWS BETWEEN CURRENT ROW AND 18446744073709551615 FOLLOWING) s
   FROM t ORDER BY a LIMIT 2;
   ```
   
   ```
   thread 'tokio-rt-worker' panicked at datafusion/physical-optimizer/src/limit_pushdown_past_window.rs:198:17:
   attempt to add with overflow
   ```
   
   `LimitPushPastWindows` grows the limit by the window's lookahead (`fetch + ctx.lookahead`, `limit_pushdown_past_window.rs:198-199`) and pushes the result into the `SortExec` below the window as a TopK fetch. The lookahead here is the frame's end bound, `u64::MAX`, so the addition overflows. In release builds it wraps instead, which would give the `SortExec` a fetch smaller than the query needs. I have not run the release build.
   
   ### To Reproduce
   
   Run the query above with a debug build, for example in `datafusion-cli` or a sqllogictest file.
   
   ### Expected behavior
   
   The query returns
   
   ```
   1 15
   2 14
   ```
   
   ### Additional context
   
   - `fetch + limit.skip()` at `limit_pushdown_past_window.rs:206` has the same problem for a huge `OFFSET` plus `LIMIT`.
   - The sort pushdown in `EnsureRequirements` already saturates the same kind of sum. `input_fetch` in `ensure_requirements/enforce_sorting/sort_pushdown.rs` uses `saturating_add`, with a comment explaining why the result stays correct.
   - A fix could use `saturating_add` at these two places, with a sqllogictest case in `window_limits.slt`.
   


-- 
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.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: [I] `LimitPushPastWindows` panics with "attempt to add with overflow" for a large `ROWS ... FOLLOWING` bound [datafusion]

["marvelefe (via GitHub)" <gi...@apache.org>]

marvelefe commented on issue #26658:
URL: https://github.com/apache/datafusion/issues/26658#issuecomment-6396817353

   Hi @jayzhan211, I'd like to pick this up. I traced the overflow to the unchecked additions in datafusion/physical-optimizer/src/limit_pushdown_past_window.rs: `fetch + ctx.lookahead` (L198/L199), `fetch + limit.skip()` (L206), and `ctx.lookahead + max_rel` (L180). I'd switch these to `saturating_add` (matching sort_pushdown.rs) and add a sqllogictest case in window_limits.slt with a large `ROWS ... FOLLOWING`. Prefer saturation, or an explicit planner error on overflow? Could you assign me?
   


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