andygrove opened a new issue, #6187: URL: https://github.com/apache/datafusion-comet/issues/6187
#6163 deprecated `spark.comet.exec.memoryPool.fraction`. This issue collects the follow-up work. ### 1. The new text describes `fair_unified` as a per-consumer limit The config doc ([CometConf.scala#L950-L952](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/comet/CometConf.scala#L950-L952)) and the memory management guide ([memory_management.md#L206-L207](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/docs/source/contributor-guide/memory_management.md?plain=1#L206-L207)) say that `fair_unified` limits each memory consumer in a task to `fraction * spark.memory.offHeap.size / num_consumers`. In fact the pool caps the task's combined reservations at that value. `CometFairMemoryPool::try_grow` compares the pool-wide `state.used` against `pool_size / num` ([fair_pool.rs#L161](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/native/core/src/execution/memory_pools/fair_pool.rs#L161)), which is #5961. The memory management guide says as much a few sections later ("It is not a per-consumer quota", [m emory_management.md#L268-L282](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/docs/source/contributor-guide/memory_management.md?plain=1#L268-L282)), so the page now contradicts itself. Both guides also say Spark's per-task limit is the tighter one "whenever more than one task is running" ([memory_management.md#L207-L209](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/docs/source/contributor-guide/memory_management.md?plain=1#L207-L209), [tuning.md#L114-L117](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/docs/source/user-guide/latest/tuning.md?plain=1#L114-L117)), which also rests on the per-consumer reading. With the real cap of `fraction * offHeap / num_consumers`, Spark's `offHeap / N` is tighter only when `N * fraction > num_consumers`. The reason for the deprecation still holds: the fraction applies per task, so the tasks together can still take the whole pool. Suggested wording: "caps the combined reservations of the task's consumers at `memory_limit / num_consumers` (see #5961)". If #5961 is fixed first and restores per-consumer limits, the current wording becomes correct instead. The tuning guide's older pool type description has the same problem, predating #6163: it says `fair_unified` prevents operators from each using more than `pool_size / num_reservations` ([tuning.md#L135-L136](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/docs/source/user-guide/latest/tuning.md?plain=1#L135-L136)). ### 2. The deprecation warning misses session-level settings `warnIfMemoryPoolFractionSet` checks the SparkConf in the driver plugin ([Plugins.scala#L186](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/spark/Plugins.scala#L186)), but the value is read per query from the session conf ([CometExecIterator.scala#L618](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/comet/CometExecIterator.scala#L618)). A value set with `spark.conf.set` or `SET` gets no warning, and neither does an application that registers the session extension without the plugin (#6186). ### 3. Tests rely on the fraction to force spills `CometTaskMetricsSuite` sets the fraction to `0.002` to force native spills ([L145](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/test/scala/org/apache/spark/sql/comet/CometTaskMetricsSuite.scala#L145), [L232](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/test/scala/org/apache/spark/sql/comet/CometTaskMetricsSuite.scala#L232), [L579](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/test/scala/org/apache/spark/sql/comet/CometTaskMetricsSuite.scala#L579)). Removing the config needs another way to force spills in tests. Alternatively, move it to the testing category instead of removing it. -- 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]
