peterxcli commented on issue #5686: URL: https://github.com/apache/datafusion-comet/issues/5686#issuecomment-5543566525
End-to-end release A/B changes the conclusion from the native-only microbenchmark: the current blanket read-side coalescing patch should not be published. Setup: macOS arm64 release build, Spark 4.1.3, AQE off, native shuffle/direct read, the same patched binary with `datafusion.execution.coalesce_batches` toggled, one warmup per mode, alternating measured runs, and exact result checks. | Consumer / shape | Coalescing off | Coalescing on | Result | |---|---:|---:|---:| | Sort, 1M rows, 128 writers / 128 reducers (3 runs), wall | 647 ms | 722 ms | 11.6% slower | | Sort, same shape, native operator | 248 ms | 276 ms | 11.3% slower | | Sort, 1M rows, 1024 writers / 1 reducer (7 runs), wall | 492 ms | 476 ms | effectively neutral | | Sort, same shape, native operator | 54 ms | 53 ms | effectively neutral | | Final grouped SUM/COUNT, 8,388,608 rows, 512 writers / 8 reducers (7 runs), wall | 116 ms | 116 ms | neutral | | Final aggregate, same shape, native operator | 28 ms | 25 ms | 10.7% operator-only gain | | Final grouped SUM/COUNT, 8,388,608 rows, 2048 writers / 8 reducers (11 runs), wall | 371 ms | 363 ms | 2.2% faster | | Final aggregate, same shape, native operator | 102 ms | 99 ms | 2.9% faster | The fragmentation is real: each Comet shuffle frame becomes one native `RecordBatch`, and writer-side coalescing cannot combine tails from different map tasks. But a read-side wrapper does not remove JNI pulls, IPC decode/decompression, dictionary unpacking, or schema conversion. For Sort, DataFusion already buffers and bulk-concatenates inputs when the partition is below its default 1 MiB in-place threshold. The extra wrapper duplicates that work and regresses the common small-reducer case. Final grouped HashAggregate has no equivalent concat shortcut, so it can save per-batch dispatch, but even a 2048-writer stress case produced only a 2.2% wall improvement. There is also a production risk: DataFusion's generic `CoalesceBatchesExec` is deprecated and its Arrow `BatchCoalescer` builders are not tracked by a DataFusion `MemoryReservation`. Applying it to variable-width aggregate state could hide significant memory and perturb spilling. Decision: I am not opening the implementation PR from this experiment. A future patch should first demonstrate a stable stage-level win (not just operator time) on a real workload and use a memory-accounted/adaptive mechanism that avoids the small-Sort regression. -- 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]
