alamb opened a new issue, #25902: URL: https://github.com/apache/datafusion/issues/25902
### Is your feature request related to a problem or challenge? - Follow on to https://github.com/apache/datafusion/issues/22710 - Earliest release this can land in: DataFusion `57.0.0` (https://github.com/apache/datafusion/issues/25901) @2010YOUY01 and others refactored `GroupedHashAggregateStream` into dedicated per-mode streams as part of - #22710 The old stream was kept behind [`datafusion.execution.enable_migration_aggregate = false`](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/common/src/config.rs#L969-L978) as a fallback in case the new streams had bugs. The [config option documentation](https://datafusion.apache.org/user-guide/configs.html) says that the fallback will be removed after the `56.0.0` release. Until it is removed, DataFusion carries two implementations, meaning: - There are two code paths that must be kept correct, which makes the aggregate code larger and harder to understand and review. - Bug fixes and features (spilling, metrics, memory accounting) have to be applied twice or explicitly excluded from the legacy path. (e.g. #24889, #24523, #25383) - Users who set `enable_migration_aggregate = false` to work around a `55.x` bug silently keep running the old, unmaintained implementation. ### Describe the solution you'd like After `56.0.0` is released, delete the legacy stream and the config option, and note the removal in the upgrade guide. The 56.0.0 release gives users one release with the fallback available, so any remaining regressions in the new streams can be reported and fixed before the fallback disappears. <details> <summary>Things to remove</summary> Code: - `datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs` - `StreamType::GroupedHash` and the fallback branch in `AggregateExec::execute_typed` in `datafusion/physical-plan/src/aggregates/mod.rs` - `ExecutionOptions::enable_migration_aggregate` in `datafusion/common/src/config.rs` and its entry in `docs/source/user-guide/configs.md` Docs: - Section "6. Legacy grouped hash aggregation" of the `aggregates` module docs - Add an entry to `docs/source/library-user-guide/upgrading/` for the removed config option </details> ### Describe alternatives you've considered Keep the fallback for another release. This delays the cleanup and means every aggregate change during that time still has to consider the legacy path. ### Additional context - #25537 asks that the spill driver unification land before the legacy stream is deleted, because the aggregation fuzzer uses the legacy stream as a differential oracle. Once the legacy stream is gone the fuzzer baseline needs another reference (for example single partition, no spilling, new streams). -- 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]
