apache / apache/datafusion-comet
Make fair_unified account for spillable consumers
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
### 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.
Contributor guide
Research direction
Start by locating the fair_unified accounting and the ExternalSorter and ExternalSorterMerge consumers. Compare the behavior with DataFusion's FairSpillPool model, then run a focused mixed-consumer test and a native sort test. Done means spillable and unspillable reservations are accounted for separately while JNI acquisition, release, rollback, reserved(), and memory_limit() remain correct; also run the proposed warmed A/B workloads.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- performance
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100