Phoenix500526 commented on PR #24923:
URL: https://github.com/apache/datafusion/pull/24923#issuecomment-5631812430
> @Phoenix500526
>
> The spill loop turns a memory shortfall into a hard failure, where the old
code deliberately did not:
>
> ```rust
> // before
> let reservation_failed = self.reservation.try_grow(sorted_size).is_err();
> // Even if the reservation is not enough, the batch is already in
> // memory, so it's okay to combine it with previously sorted batches
> globally_sorted_batches.push(batch);
> if reservation_failed { self.consume_and_spill_append(...)?; }
>
> // after (sort.rs:517)
> let workspace = self.merge_pool.borrow(sorted_size);
> self.reservation.try_grow(sorted_size - workspace.size())?; // <-- new
failure path
> ```
>
> That old comment is still true and the new code contradicts it. At this
point the batch was just produced by `sorted_stream.next()`, so it is _already
resident_. Returning `ResourcesExhausted` reclaims nothing — it only kills a
query that would otherwise have completed. Refusing an allocation is the right
call when refusing prevents it; here it doesn't.
>
> This is a real regression, not a hypothetical: the PR relaxes an existing
test to keep it green —
>
> ```diff
> - .with_sort_spill_reservation_bytes(1),
> + .with_sort_spill_reservation_bytes(spill_workspace),
> - .with_memory_limit(batches_memory, 1.0)
> + .with_memory_limit(batches_memory + spill_workspace, 1.0)
> ```
>
> — and `test_spill_output_respects_memory_limit` then asserts that the
_old_ configuration now yields `ResourcesExhausted`. Users running a tight
`sort_spill_reservation_bytes`, or sorting wide `Utf8View`/string batches where
one output batch exceeds the remaining merge workspace, get a new OOM on a
workload that spilled fine before.
>
> Please keep the workspace-borrow optimization but stop propagating the
shortfall:
>
> ```diff
> - let spill_workspace = match
self.reservation.try_grow(sorted_size) {
> - Ok(()) => None,
> - Err(_) => {
> - let workspace = self.merge_pool.borrow(sorted_size);
> - self.reservation.try_grow(sorted_size -
workspace.size())?;
> - Some(workspace)
> - }
> - };
> + let spill_workspace = match
self.reservation.try_grow(sorted_size) {
> + Ok(()) => None,
> + Err(_) => {
> + // Reuse already-reserved workspace where we can.
> + let workspace = self.merge_pool.borrow(sorted_size);
> + let remainder = sorted_size - workspace.size();
> + // The batch is already in memory: failing here would
not
> + // reclaim it, so record the usage and spill it
immediately.
> + if self.reservation.try_grow(remainder).is_err() {
> + self.reservation.grow(remainder);
> + }
> + Some(workspace)
> + }
> + };
> ```
>
> With that, `test_spill_output_respects_memory_limit` should be dropped and
the two config changes in
`should_return_stream_with_batches_in_the_requested_size_and_update_metrics_when_having_to_spill`
reverted. If you believe the strict behaviour is genuinely the one we want, it
needs to be called out in the PR description and in the upgrade guide as a
breaking behavioural change — it shouldn't land as a side effect of adding an
async writer API.
Hi, @jayzhan211 thanks for pointing this out. In
[a979e98fc](https://github.com/apache/datafusion/commit/a979e98fc177db4733d2758876bf074be5aeb24e),
I restored the existing spill fallback behavior while retaining workspace
reuse and memory accounting across async writes. I also removed the
strict-failure test and restored the original memory settings in the existing
spill test. I opened
[#25183](https://github.com/apache/datafusion/issues/25183) to track the
memory-limit guarantees and potential compatibility changes separately. We can
discuss stricter enforcement there if needed, keeping this PR focused on async
spill writing.
--
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]
