peterxcli opened a new issue, #5465: URL: https://github.com/apache/datafusion-comet/issues/5465
### 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. -- 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]
