refactor(hash-aggr): Small fixes and finish hash aggregation refactor - #24961
Conversation
…tream selection Apart from the TopK path, `limit_options` is only a hint: the partial and final hash streams use it to stop reading input once enough groups are accumulated, and the downstream limit operator enforces the exact row count either way. The remaining streams do not implement the optimization yet and simply consume all input. So the soft limit does not need to take part in stream selection. Drop the limit conditions from the stream predicates instead of falling back to the legacy `GroupedHashAggregateStream` for single, partial-reduce and ordered aggregation with a soft limit. The removed `limit_options_supported_by_hash_stream()` was always true at this point anyway: a limit that is not an unordered distinct soft limit already took the TopK path. Part of apache#22710
… input `PartialReduce` aggregation has no ordered stream implementation, so plan it with `InputOrderMode::Linear` like grouping sets. Part of apache#22710
…fallback Every grouped aggregation shape now runs on a dedicated stream, so return an internal error instead of falling back to the legacy `GroupedHashAggregateStream`. The legacy implementation is kept behind `enable_migration_aggregate = false` for one more release. Part of apache#22710
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24961 +/- ##
==========================================
- Coverage 81.62% 81.62% -0.01%
==========================================
Files 1124 1124
Lines 412462 412487 +25
Branches 412462 412487 +25
==========================================
+ Hits 336684 336693 +9
- Misses 55965 55974 +9
- Partials 19813 19820 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @2010YOUY01 , there is a suggestion
| AggregateMode::Single | AggregateMode::SinglePartitioned | ||
| ) && self.limit_options.is_none() | ||
| && self.input_order_mode == InputOrderMode::Linear | ||
| ) && self.input_order_mode == InputOrderMode::Linear |
There was a problem hiding this comment.
I think we might need group_values_soft_limit like PartialReduceHashAggregateStream for SingleHashAggregateStream
There was a problem hiding this comment.
Routing change here drops the distinct soft-limit optimization for Single/SinglePartitioned aggregates.
On main, should_use_single_hash_stream required limit_options.is_none(), so a distinct aggregate with lim=[n] fell through to the legacy GroupedHashAggregateStream, which stops consuming input once n groups exist. With this change it routes to SingleHashAggregateStream, which has no group_values_soft_limit and reads the whole input. The plan text is unchanged so no slt catches it, but e.g. aggregate.slt:7290 (mode=SinglePartitioned, aggr=[], lim=[5]) now scans everything under the limit. That is the shape LimitedDistinctAggregation produces for SELECT DISTINCT ... LIMIT n on single-partition / hash-partitioned inputs.
There was a problem hiding this comment.
This looks like a possible optimization that was missing from the existing implementation. Otherwise existing tests will complain (this kind of optimization usually come with tests asserting plan shape)
There was a problem hiding this comment.
Probably, let me file an issue to track this
3 participants
Which issue does this PR close?
There are some smaller cleanups remaining, and I plan to move them to new issue afterwards.
Rationale for this change
See EPIC for the background.
This PR first implements two smaller fixes, both for aggregate plan routing; it's easier to figure out the changes by reading the code diffs of commit 1 and commit 2. They're both verified by existing tests -- no test needed updating, which means no existing query plans are affected.
The 3rd commit soft-deletes the legacy implementation: all aggregations will be planned by the new code; otherwise an internal error will be triggered.
The legacy implementation and the config
enable_migration_aggregateare planned to be kept for 1 more release, in case there are major bugs, so users can work around them by reverting via the config. After 1 release, the legacy implementation should be good to delete. Since this is an internal refactor rather than a public API change, it might be good to use such an informal deprecation protocol.What changes are included in this PR?
What is the testing strategy for this PR?
Existing tests
Are there any user-facing changes?
No