What is the problem the feature request solves?
fair_unified currently gives every registered memory consumer equal weight when it calculates a fair share. As a result, unspillable consumers reduce the share available to operators that can actually respond to memory pressure by spilling.
DataFusion's FairSpillPool handles the two kinds of consumers separately: it subtracts memory held by unspillable consumers, then divides the remainder among spillable consumers.
This difference appears in normal native sorts. ExternalSorter is spillable, while ExternalSorterMerge is not, but both currently increase Comet's divisor.
The focused fix for #5212 (finding #1) deliberately leaves this policy unchanged. It only fixes the reservation used in the current fair-share check. Spillability-aware accounting should be evaluated separately so that a policy change is not mixed into that root-cause fix.
Describe the potential solution
Match DataFusion's accounting model:
- Track spillable and unspillable consumers and their reserved bytes separately.
- For a spillable request, divide the memory left after unspillable reservations among the spillable consumers.
- Admit unspillable requests only while aggregate reservations remain within the pool limit.
- Preserve the existing JNI acquisition, release, and partial-grant rollback behavior.
- Keep
reserved() and memory_limit() truthful.
This needs the full mixed-consumer accounting, not just a different divisor. For example, with a 32 MiB pool and a 10 MiB merge reservation, giving the sorter a full 32 MiB share would admit 42 MiB.
A focused test should cover mixed spillable and unspillable reservations, and a native sort test should show that merge memory reduces the sorter's available share.
Additional context
The per-reservation fix confirms that the low-concurrency path is reachable:
- 200k rows: 5 spills on main, 2 after the fix.
- 2m rows: 43 spills on main, 16 after the fix.
- 2m spilled bytes: 232,467,592 → 130,125,664.
The timing ranges overlapped, so this is spill and capacity evidence, not a speed claim. We have not run a spillable-only A/B yet, and the remaining spills do not prove that this policy change will help.
Fair-pool policy is performance-sensitive: #1369 reported TPC-H q3 slowing from about 16 seconds to 3.4 minutes. Before implementing this, we should run warmed repeated A/B on the same tiny and representative low-concurrency workloads and compare spill count, spilled bytes, capacity, and task tail. If that does not show a material improvement in a real workload, we should leave the current policy alone.
What is the problem the feature request solves?
fair_unifiedcurrently gives every registered memory consumer equal weight when it calculates a fair share. As a result, unspillable consumers reduce the share available to operators that can actually respond to memory pressure by spilling.DataFusion's
FairSpillPoolhandles the two kinds of consumers separately: it subtracts memory held by unspillable consumers, then divides the remainder among spillable consumers.This difference appears in normal native sorts.
ExternalSorteris spillable, whileExternalSorterMergeis not, but both currently increase Comet's divisor.The focused fix for #5212 (finding #1) deliberately leaves this policy unchanged. It only fixes the reservation used in the current fair-share check. Spillability-aware accounting should be evaluated separately so that a policy change is not mixed into that root-cause fix.
Describe the potential solution
Match DataFusion's accounting model:
reserved()andmemory_limit()truthful.This needs the full mixed-consumer accounting, not just a different divisor. For example, with a 32 MiB pool and a 10 MiB merge reservation, giving the sorter a full 32 MiB share would admit 42 MiB.
A focused test should cover mixed spillable and unspillable reservations, and a native sort test should show that merge memory reduces the sorter's available share.
Additional context
The per-reservation fix confirms that the low-concurrency path is reachable:
The timing ranges overlapped, so this is spill and capacity evidence, not a speed claim. We have not run a spillable-only A/B yet, and the remaining spills do not prove that this policy change will help.
Fair-pool policy is performance-sensitive: #1369 reported TPC-H q3 slowing from about 16 seconds to 3.4 minutes. Before implementing this, we should run warmed repeated A/B on the same tiny and representative low-concurrency workloads and compare spill count, spilled bytes, capacity, and task tail. If that does not show a material improvement in a real workload, we should leave the current policy alone.