andygrove opened a new pull request, #6191: URL: https://github.com/apache/datafusion-comet/pull/6191
## Which issue does this PR close? Closes #6183. Closes #6184. Closes #6185. ## Rationale for this change An audit of Comet's memory-related configs found three settings that size native memory or disk use being read in the wrong unit: - `spark.comet.shuffle.native.writeBufferSize` was declared in MiB, but its value was sent to native code as a byte count. The native shuffle writer therefore ran with a 1-byte write buffer by default, which disabled buffering of shuffle and spill file writes, the scratch buffer reuse from #5568, and the single-read spill copy from #5916. - `spark.comet.maxTempDirectorySize` reached native code as the raw string the user set, and native code parses only a bare integer. Any value with a unit silently fell back to 100 GiB. The boolean flags native code reads had the same problem with anything other than lowercase `true`. - `spark.memory.offHeap.size` was read as MiB when sizing the memory pool, while Spark reads a bare number as bytes. A bare byte count made the `fair_unified` pool's per-task cap effectively unlimited. ## What changes are included in this PR? - `spark.comet.shuffle.native.writeBufferSize` is now a byte size with a 1 MiB default. A bare number keeps its current meaning, since native code already treated it as bytes. The default, and values with a unit such as `8m`, now mean what they say. - `CometExecIterator.serializeCometSQLConfs` resolves every config that native code reads and sends the resolved value, including defaults. That covers `maxTempDirectorySize` and the `debug.enabled`, `debug.memory`, `explain.native.enabled` and `tracing.enabled` flags, alongside the existing `parquet.rowFilterPushdown.enabled`. An invalid value now fails with the usual config error instead of being silently replaced. - The `maxTempDirectorySize` docs say the limit applies per native plan rather than per task. Each native plan gets its own disk manager, and a task can run more than one plan. - `getMemoryConfig` reads `spark.memory.offHeap.size` as bytes, as the memory usage log in the same file already did. With the write buffer fixed, each shuffle-writing task holds up to about 3 MiB of buffers that no memory pool tracks, which is what the setting always intended. ## How are these changes tested? - New `CometNativeShuffleWriterSuite` checks the write buffer size in the plan sent to native code for the default, for `8m`, and for a bare byte count. It is registered in both PR workflows. - A new `CometExecSuite` test checks that the configs native code parses are serialized resolved, both with and without explicit settings. - A new `CometExecIteratorLifecycleSuite` test checks `getMemoryConfig` with a bare byte count and with `4g`. On a macOS laptop, `shuffle_bench` with TPC-H SF1 `lineitem`, 200 hash partitions and lz4 shows no difference between the two buffer sizes without spilling. With a 64 MiB memory limit (17 spills), writing is about 3% faster with a 1 MiB buffer than with a 1-byte buffer, and the write time drops from 0.192s to 0.116s. -- 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]
