andygrove opened a new issue, #6127:
URL: https://github.com/apache/datafusion-comet/issues/6127
### Describe the bug
`CometUnifiedMemoryPool` and `CometFairMemoryPool` implement
`MemoryPool::grow` as `self.try_grow(..).unwrap()` (`unified_pool.rs:111`,
`fair_pool.rs:121`), so the task panics whenever Spark grants less than the
full request. DataFusion documents `grow` as "Infallibly grow the provided
`reservation` by `additional` bytes. This must always succeed", and its own
pools implement it by adding to the counter and allowing the limit to be
exceeded. Callers use `grow`, not `try_grow`, when the memory already exists
and only needs to be recorded, so they have no way to handle a refusal. With
Comet's pools the task dies and Spark retries it.
The panic was a deliberate trade-off. #1733 found that the old `grow`
ignored a partial grant from Spark, so `shrink` could release more than was
acquired. Of the two fixes proposed there, #1732 (panic when the grant falls
short) was merged instead of #1731 (track what was actually acquired). That
fixed the over-release but broke the `grow` contract.
### Steps to reproduce
TPC-H SF100 on Spark 4.1.1, 2 executors x 8 cores,
`spark.memory.offHeap.size=2g`, default `fair_unified` pool. Three tasks in one
stage fail with:
```
Comet native panic: panicked at
core/src/execution/memory_pools/fair_pool.rs:122:49:
called `Result::unwrap()` on an `Err` value: ResourcesExhausted("Failed to
acquire 327992 bytes, only got 283353 bytes. Reserved: 357630588 bytes")
at datafusion_execution::memory_pool::MemoryReservation::grow
at
datafusion_physical_plan::joins::sort_merge_join::materializing_stream::MaterializingSortMergeJoinStream::restore_spilled_batches
```
The sort-merge join calls `grow` for a spilled batch it has just read back
from disk. That memory already exists, so there is nothing to refuse.
### Expected behavior
`grow` never fails:
- It acquires what Spark will grant.
- It records any shortfall as overcommit, so `reserved()` still reflects the
full amount in use.
- `shrink` pays down the overcommit before returning bytes to Spark, so
Spark is never given back more than it granted. This keeps the #1733 fix.
A later `try_grow` still sees the full reserved amount, so the operator that
asks next is refused and spills.
### Additional context
- #2453 (releasing more memory than allocated) may share a root cause.
- Found while working on #6125, where the Parquet scan has to use `try_grow`
and fall back to unaccounted memory because an infallible `grow` would panic.
--
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]
