jayzhan211 commented on PR #24923:
URL: https://github.com/apache/datafusion/pull/24923#issuecomment-5620074706
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.
--
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]
