andygrove opened a new issue, #2060: URL: https://github.com/apache/datafusion-ballista/issues/2060
`SortShuffleWriterExec` registers its `MemoryConsumer` with `.with_can_spill(true)`, but discards the result of the reservation grow ([`writer.rs`](https://github.com/apache/datafusion-ballista/blob/main/ballista/core/src/execution_plans/sort_shuffle/writer.rs)): ```rust // Mirror the growth in the runtime pool reservation so the pool // sees this writer's memory usage. Best-effort: if the pool is // bounded and rejects the grow, that's fine — the absolute // counter below still triggers a spill. let _ = reservation.try_grow(growth); buffered_bytes = buffered_bytes.saturating_add(growth); if buffered_bytes >= memory_limit { // spill } ``` The comment argues the rejection is safe to ignore because `buffered_bytes` still bounds RSS. That reasoning does not hold: `buffered_bytes` is a private per-task counter compared against `memory_limit_per_task_bytes`. It knows nothing about the state of the shared `MemoryPool`, so a rejected grow produces **no spill at all**. Two consequences follow. **1. The writer under-reports its usage to the shared pool.** The batch has already been pushed into `BufferedBatches` by this point. When the grow is rejected, the writer holds that memory while the pool has no record of it, so other consumers sharing the pool see bytes as available that are not. This is the same class of accounting gap as #2031, but self-inflicted rather than caused by an untracked allocator. **2. Pool pressure produces no spill.** Registering with `can_spill(true)` is a contract: the pool rejects a grow to ask the consumer to spill. This writer declines to respond, so the only thing that can ever trigger a spill is its own per-task budget. This directly undermines the approach proposed in #2031, whose remedy is to *\"reject memory-pool growth before real usage exceeds the budget, so DataFusion spills instead of OOMing\"*. That guard cannot work on this operator: the writer buffers straight through a rejection. ### Reproduction Give the writer a per-task budget far above the payload (so the private counter can never fire) and a pool too small to admit a single batch, so every grow is rejected. The writer spills zero times and buffers the entire input. ### Proposed fix Treat a rejected grow as a spill trigger alongside the existing per-task budget. Spilling flushes the buffered batches to disk and frees the reservation, so no retry is needed — the writer holds nothing afterwards. -- 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]
