andygrove opened a new pull request, #25805:
URL: https://github.com/apache/datafusion/pull/25805

   ## Which issue does this PR close?
   
   - Part of #25758
   - Part of #25804
   
   ## Rationale for this change
   
   On `branch-55`, a sort that spills can fail with `ResourcesExhausted` for 
`ExternalSorterMerge[N]` when the memory pool is shared, even though it could 
have spilled instead. `ExternalSorter` releases the 
`sort_spill_reservation_bytes` headroom to the pool before the in-memory merge 
it runs while spilling, and `MultiLevelMergeBuilder` releases it again after 
each intermediate merge pass, so another consumer can take that memory before 
the merge needs it.
   
   DataFusion Comet, which uses 55.1.0, loses tasks to this in TPC-H and TPC-DS 
runs (apache/datafusion-comet#2452, apache/datafusion-comet#5666). These are 
findings 1 and 2 in #25804.
   
   ## What changes are included in this PR?
   
   This PR backports #24740 from @sunchao to the `branch-55` line. The 
cherry-pick applied without conflicts.
   
   #24740 adds `MergeMemoryPool`, which keeps the merge headroom charged to the 
sorter's merge consumer across spills and merge passes, and lets the merge's 
cursor, row, and batch reservations draw on it.
   
   Note for the release manager: `MergeMemoryPool` and `WorkspaceLoan` are new 
public types in `datafusion-execution`. The fix depends on them, and the change 
is additive.
   
   ## Are these changes tested?
   
   Yes. The backport includes the tests from #24740 
(`sorts/sort/spill_tests.rs` and the `MergeMemoryPool` unit tests). On this 
branch I ran:
   
   - `cargo test -p datafusion-execution --lib` and `cargo test -p 
datafusion-physical-plan --lib`
   - `cargo test -p datafusion --test core_integration memory_limit`
   - `cargo test -p datafusion --features extended_tests --test fuzz` for 
`sort_fuzz`, `spilling_fuzz`, and `sort_query_fuzz`
   - `cargo clippy --all-targets --all-features -- -D warnings`
   
   The `merge_headroom_is_lost_after_first_pass` reproduction in #25804 fails 
its second merge pass on `branch-55`. With this backport, the merge completes.
   
   ## Are there any user-facing changes?
   
   Sorts that spill no longer fail when another consumer of a shared pool takes 
the merge headroom. `MergeMemoryPool` and `WorkspaceLoan` are added to the 
public API of `datafusion-execution`.
   


-- 
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]

Reply via email to