apache / apache/datafusion-comet

Make fair_unified account for spillable consumers

Open
#5,465 0 comments 0 reactions 0 assignees View on GitHub
enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.