hhhizzz opened a new pull request, #24853: URL: https://github.com/apache/datafusion/pull/24853
## Which issue does this PR close? - Closes #24849. ## Rationale for this change #23182 fixed a performance regression in terminal hash aggregate output by avoiding repeated destructive `EmitTo::First` calls. However, the remaining `MaterializedAggregateOutput` unit test only exercises the slicing helper and does not verify the behavior through `AggregateExec`. A result-only integration test would pass with both the regressed and fixed implementations, while a wall-clock assertion would be unreliable in CI. This PR therefore uses a test `GroupsAccumulator` that returns an error if terminal output falls back to destructive `EmitTo::First`. ## What changes are included in this PR? - Adds `single_grouped_aggregate_avoids_destructive_terminal_drain`, which runs unordered Single hash aggregation with more groups than the configured `batch_size`. - Verifies that `AggregateExec` selects `SingleHash`, emits multiple bounded batches, and returns the correct aggregate results for repeated group keys. - Extends the test accumulator with preserving evaluation and state support so the regression test remains compatible with future direct blocked or chunked output implementations. - Keeps the test independent of `MaterializedAggregateOutput`; Single and Final hash aggregation exercise the same terminal drain helper. ## What is the testing strategy for this PR? - Verified that temporarily restoring destructive `EmitTo::First(batch_size)` causes the new test to fail with the expected accumulator error, while the current implementation passes. - Applied an equivalent FinalHash accumulator oracle to the historical regression: it fails on `d58e0c6d4^` and passes on `d58e0c6d4`. The migrated SingleHash stream did not exist at that historical revision. - Ran `cargo fmt --all`. - Ran `cargo clippy --all-targets --all-features -- -D warnings`. - Ran the extended workspace test suite with `avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`; all tests passed after initializing the required Arrow and Parquet test fixtures. ## Are there any user-facing changes? No. This PR only adds regression test coverage. -- 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]
