andygrove opened a new pull request, #6209: URL: https://github.com/apache/datafusion-comet/pull/6209
Backport of #6191 to `branch-1.0`. Cherry-picked from `02e84047a423a08debe3c6b6daab19fd9233427c`. The fixes themselves are unchanged. Five adaptations were needed, described under "What changes are included" below. Two of them touch production code, and neither changes behaviour: a visibility widening and an unused import. ## Which issue does this PR close? Closes #6183, #6184 and #6185 on `branch-1.0`. Listed in #6201. ## Rationale for this change All three bugs ship in 1.0.0, through the same code as on `main` before #6191: - `spark.comet.shuffle.native.writeBufferSize` is declared in MiB but sent to native code as a byte count, so the native shuffle writer runs with a 1-byte write buffer by default. On `branch-1.0` the setting sizes the multi-partition data file's `BufWriter` and the flush threshold of every `BufBatchWriter`, so each encoded block goes straight to its own write call, for shuffle and spill files alike. - `spark.comet.maxTempDirectorySize` reaches native code as the raw string, and native code parses only a bare integer, so a value with a unit silently falls back to 100 GiB. The boolean flags native code reads have the same problem with anything other than lowercase `true`. Native code on `branch-1.0` reads the same configs as on `main`. - `spark.memory.offHeap.size` is read as MiB when sizing the memory pool, while Spark reads a bare number as bytes, so a bare byte count makes the `fair_unified` per-task cap effectively unlimited. ## What changes are included in this PR? The fix is the original one, so see #6191 for the details. The adaptations: - CI suite lists in `pr_build_linux.yml` and `pr_build_macos.yml`: added only `CometNativeShuffleWriterSuite`. Its neighbours on `main`, `CometNativePositionalRoundRobinSuite` and `CometDiskBlockWriterSuite`, come from later commits and do not exist here. - The `getMemoryConfig` test lives in `CometExecSuite`, next to this PR's other `CometExecIterator` test, instead of in `CometExecIteratorLifecycleSuite`. That suite comes from #5494, which is not on `branch-1.0`. Creating it for one test would mean another suite to register, and an add/add conflict for any later backport of #5494. The test body is unchanged, and `CometExecSuite` gains a `SparkConf` import. - `CometNativeShuffleWriter.buildUnifiedPlan` is widened from `private` to `private[shuffle]` so the new suite can call it, the same change #5513 made on `main`. On `branch-1.0` it still takes separate data and index paths (#5916 later dropped the index path on `main`), so the suite passes both. - `CometExecIterator.scala` drops its `ByteUnit` import. The `spark.memory.offHeap.size` fix removes its last use on `branch-1.0`, and the scalafix `RemoveUnused` check in the Lint Java job rejects unused imports. On `main`, #6162 still uses it. - The `spark.comet.maxTempDirectorySize` row of `docs/source/user-guide/latest/configs.md` now has the new description. `branch-1.0` commits the generated config reference, while `main` generates it at publish time, so #6191 had no copy to update. The file now matches `GenerateDocs` output exactly. The `writeBufferSize` row is unchanged, because the old MiB default already rendered as `1048576b`. ## How are these changes tested? Same tests as the original PR, verified locally on `branch-1.0` with Spark 4.1.3, Scala 2.13 and JDK 17 unless noted: - `CometNativeShuffleWriterSuite` and `CometExecSuite` pass, 146 tests including the three new ones. - The bugs are present on `branch-1.0`, and the tests catch them. With the `CometConf.scala` and `CometExecIterator.scala` changes reverted and the tests kept, exactly the three new tests fail and the other 143 pass. The plan sent to native code carries a write buffer of `1` rather than `1048576`, the serialized configs have no `maxTempDirectorySize` entry, and a bare `4294967296` off-heap size gives a memory limit of `4503599627370496`, which is 4 PiB. - The same two suites pass on Spark 3.4.3, Scala 2.12 and JDK 11, the oldest profile `branch-1.0` CI runs: 143 passed, and 3 were cancelled by existing Spark version guards. - `CometNativeShuffleSuite`, `CometShuffleSuite` and `CometShuffleEncryptionSuite` pass, 73 tests, now that the native shuffle writer uses a 1 MiB buffer instead of 1 byte. - scalafix in CHECK mode on Spark 3.5 / Scala 2.12, Spotless, Scalastyle, `apache-rat:check`, `cargo fmt --all -- --check`, `cargo clippy --all-targets --workspace -- -D warnings`, `dev/ci/check-suites.py`, `dev/ci/check-ci-config.py`, `actionlint`, and `prettier --check` on the two changed docs all pass. ## Are there any user-facing changes? Yes, the same ones as #6191, which are worth weighing for a patch release: - The native shuffle writer now uses the 1 MiB write buffer the setting documents. On `branch-1.0` each shuffle-writing task holds up to about 2 MiB of buffers that no memory pool tracks: the data file's `BufWriter` and one `BufBatchWriter` staging buffer at a time. That is about half of the 4 MiB on `main`, where #5568 and #5916 added buffers of the same size. - `writeBufferSize` values with a unit now mean what they say, and a bare number keeps its meaning. A value of 2 GiB or more, which the native `int32` field cannot hold, now fails with a config error. Previously `4g`, for example, became a 4096-byte buffer. - `maxTempDirectorySize` values with a unit, and upper-case booleans for the flags native code reads, now take effect. An invalid value for any of them now fails with the usual config error instead of silently becoming the native default. - A bare byte count for `spark.memory.offHeap.size` now sizes the Comet memory pool in bytes, as Spark does. -- 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]
