peterxcli opened a new pull request, #5723: URL: https://github.com/apache/datafusion-comet/pull/5723
## Which issue does this PR close? Closes #5717. ## Rationale for this change High-cardinality partial aggregation can spend substantial CPU maintaining a hash table while emitting almost as many rows as it consumes. Re-enable DataFusion's adaptive bypass only for supported native-shuffle plans, rather than lowering the threshold globally. DataFusion 55 requires `convert_to_state`, but several Comet accumulators still return an unsupported-operation error. Spark's grouping-only DISTINCT stages also need an explicit mode to preserve full deduplication. ## What changes are included in this PR? - Serialize distribution-required grouping-only aggregates as native `Final` to preserve DISTINCT semantics. - Admit adaptive skipping only for native shuffle-writer plans whose partial aggregates are grouping-only or single-argument COUNT. An unsupported partial accumulator, PartialMerge, or mixed mode disables skipping throughout the fused plan. Non-native-shuffle roots remain disabled. - Apply the eligibility guard after DataFusion configuration pass-through, so testing overrides cannot bypass safety. Eligible plans retain the adaptive default and explicit disablement remains available. - Expose `skipped_aggregation_rows` as a SQL metric and document the conservative scope. No new configuration, dependency, or execution wrapper is introduced. ## How are these changes tested? - Full `CometAggregateSuite`: 93 passed, zero failures, 2 ignored. The two new focused regressions also passed independently. - Native `skip_partial_eligibility_is_fail_closed`: 1 passed. - Native release build, Rust formatting check, Scala Spotless formatting, and diff whitespace check passed. - Separate 2.4-million-row correctness fixture: 22 exact comparisons against Spark, zero errors, covering COUNT, nullable/filtered COUNT, DISTINCT, empty/null inputs, SUM, and mixed PartialMerge. - The checked-in Spark regression asserts actual skipping for eligible high-cardinality COUNT and no skipping for low cardinality, explicit disablement, unsupported accumulators/modes, and JVM shuffle. Release-mode Spark A/B on the DataFusion 55 base (`75fdddc9285ec61c0cd326977c61dd41fca39a8b`): 16,777,216 Parquet rows, 8 writers, 32 reducers, local[8], Spark 4.1.3, native direct-read shuffle, AQE/exchange reuse disabled, one warmup per mode, alternating pairs. Build/test processes had finished before timing. | Workload | Pairs | Disabled median | Guarded enabled median | | --- | ---: | ---: | ---: | | High-cardinality COUNT | 7 | 664 ms | 479 ms | | Low-cardinality control | 7 | 199 ms | 185 ms | | High-cardinality COUNT, 128 MiB native memory | 3 | 945 ms | 858 ms | High-cardinality COUNT improved 27.9%, winning all seven pairs; enabled runs bypassed 15,925,248 rows each, versus zero when disabled. The low-cardinality control bypassed zero rows in both modes, so no optimization gain is claimed there. All eight constrained-memory executions (including warmups) returned correct results with 64 aggregate spills and approximately 273 MB spilled each. Enabled executions still bypassed 15,925,248 rows, exercising skipping and spilling together. These are workstation measurements, not a universal speedup or coverage of every memory pool, AQE, or distributed deployment. Additional benchmark context is recorded in #5717. Focused reproduction from the repository root: ```sh ./mvnw test -Dtest=none '-Dsuites=org.apache.comet.exec.CometAggregateSuite skip partial aggregation' -Dscalastyle.skip=true -Djni.dir="$PWD/native/target/release" ``` Native regression from `native/` (on macOS, expose the installed JVM's `lib/server` via `DYLD_LIBRARY_PATH`): ```sh RUSTFLAGS='-Ctarget-cpu=native' cargo test --release --locked -p datafusion-comet skip_partial_eligibility --lib ``` -- 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]
