andygrove opened a new pull request, #4985: URL: https://github.com/apache/datafusion-comet/pull/4985
## Summary The async columnar shuffle path (`spark.comet.columnar.shuffle.async.enabled`) has been disabled by default and lacks meaningful test coverage. This PR removes the feature entirely along with its supporting classes and configs. Note: this depends on the `withAlternative` config-alias machinery from #TBD (branch `config-alias-machinery`) — merge that first, then rebase this on main. ### Removed - `CometShuffleExternalSorterAsync.java` (417 lines) and `ShuffleThreadPool.java` (72 lines) - Async branches from `CometBypassMergeSortShuffleWriter.java` and `CometDiskBlockWriter.java` - Configs `spark.comet.columnar.shuffle.async.enabled` and `spark.comet.columnar.shuffle.async.thread.num` - Test suites `CometAsyncShuffleSuite`, `DisableAQECometAsyncShuffleSuite`, and the `asyncShuffleEnable` machinery in `CometColumnarShuffleSuite` - The "Comet Async JVM Shuffle" case from `CometShuffleBenchmark` - The async config table row in `docs/source/contributor-guide/jvm_shuffle.md` ### Simplified - `CometShuffleExternalSorter` was an interface with a `create` factory that returned either the sync or async implementation. With only sync left, `CometShuffleExternalSorterSync` is merged back into `CometShuffleExternalSorter` as a single concrete class. Callers now use `new CometShuffleExternalSorter(...)`. - `CometDiskBlockWriter.doSpill(boolean forceSync)` becomes `doSpill()` — the `forceSync` parameter existed only to distinguish sync vs async paths. ### Renamed `spark.comet.columnar.shuffle.async.max.thread.num` → `spark.comet.columnar.shuffle.max.writers.per.executor`. Despite its old name, this config was actually a cap on the number of hash-shuffle writers per executor (partitions × cores), used in `CometShuffleManager.shouldBypassMergeSort` to avoid OOM. That cap is still useful without async. The old key is kept as an alias via `withAlternative`. Net: -773 lines across 14 files. ## Test plan - [x] `./mvnw -pl spark -am compile -Pspark-3.5` — passes - [x] `./mvnw -pl spark -am test-compile -Pspark-3.5` — passes - [x] `./mvnw spotless:check` — passes - [x] `CometShuffleManagerSuite` — 1/1 pass (exercises the renamed config) - [x] `DisableAQECometShuffleSuite` — 43/43 pass - [x] `CometConfSuite` — 7/7 pass (exercises the `withAlternative` alias path) - [ ] CI full test suite -- 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]
