andygrove opened a new issue, #7092: URL: https://github.com/apache/datafusion-comet/issues/7092
### Describe the bug DataFusion's `BoundedWindowAggExec` registers no `MemoryConsumer`, so the rows it keeps for its window frames are invisible to Comet's memory pool and to Spark. #6253 covered both window operators. #6279 reserves what `WindowAggExec` buffers by replacing it with `CometWindowAggExec`, which leaves `BoundedWindowAggExec` as the one native window operator that reserves nothing. The planner uses `BoundedWindowAggExec` when every window expression can run in bounded memory, such as `ROW_NUMBER`, `RANK`, `LEAD`/`LAG`, and aggregates over frames like `ROWS BETWEEN 3 PRECEDING AND CURRENT ROW`. It keeps only the rows its frames still need, so the amount is bounded by the frame rather than the whole task's input. That bound is small for most frames, but it isn't a fixed size: - A `RANGE ... CURRENT ROW` frame includes every peer row, meaning every row with the same `ORDER BY` value. A heavily skewed `ORDER BY` value can therefore keep a large group of rows, and the operator holds them without a reservation. - A frame that starts at `UNBOUNDED PRECEDING` and is evaluated incrementally keeps whatever its state needs for the window partition so far. Spark's `WindowExec` buffers through `ExternalAppendOnlyUnsafeRowArray`, which spills. The native path grows until the executor hits its container limit, and the pool never gets a chance to refuse anything. ### Steps to reproduce I haven't measured this. A traced run (`spark.comet.tracing.enabled=true`) of `SUM(v) OVER (ORDER BY k RANGE BETWEEN CURRENT ROW AND CURRENT ROW)` over data where one value of `k` covers most of the rows should show `native_allocated` running ahead of the pools' reserved total by about the size of that group. ### Expected behavior `BoundedWindowAggExec` reserves the rows it keeps. A group of rows that doesn't fit then fails the task with a memory error, or spills once DataFusion supports it, instead of growing unaccounted. ### Additional context - #6253 is the original report, and #6279 is the fix for `WindowAggExec`. - Spilling for the window operators is apache/datafusion#22946. - `CometWindowAggExec` in `native/core/src/execution/operators/window_agg.rs` shows how the whole-partition operator counts its buffered batches with `RecordBatchMemoryCounter`. `BoundedWindowAggExec` keeps its rows in per-partition state rather than a list of batches, so the same approach may need a wrapper or an upstream change in DataFusion rather than a copy of that stream. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
