Refactor groupby aggregation, removing simple_aggregations_collector and aggregation_finalizer classes
#21064
+550
−1,246
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ttnghia
added
libcudf
There was a problem hiding this comment.
Pull request overview
This PR refactors the groupby aggregation system by removing visitor pattern base classes (simple_aggregations_collector and aggregation_finalizer) and replacing them with template functors that use explicit specializations. This eliminates boilerplate code from aggregation class definitions and localizes special handling logic.
Changes:
- Removed visitor pattern infrastructure and virtual methods from aggregation classes
- Introduced template functors with specializations for preprocessing and finalizing aggregations
- Replaced visitor dispatch with
cudf::detail::aggregation_dispatcher()calls
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/rolling/detail/rolling.cuh | Refactored rolling window preprocessing/postprocessing from visitor classes to template functors with specializations |
| cpp/src/groupby/hash/hash_compound_agg_finalizer.hpp | Changed from visitor class to context struct plus template functor with specialization declarations |
| cpp/src/groupby/hash/hash_compound_agg_finalizer.cu | Implemented template functor specializations for compound aggregation finalization |
| cpp/src/groupby/hash/extract_single_pass_aggs.cpp | Replaced visitor class with template functor for collecting simple aggregations |
| cpp/src/groupby/hash/compute_groupby.cu | Updated to use new functor-based dispatch instead of visitor pattern |
| cpp/src/aggregation/aggregation.cpp | Removed visitor pattern base class implementations (400+ lines of boilerplate) |
| cpp/include/cudf/detail/aggregation/aggregation.hpp | Removed visitor base class declarations and virtual method declarations from aggregation classes |
| cpp/include/cudf/aggregation.hpp | Removed forward declarations of visitor classes and virtual method declarations from base aggregation class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
ttnghia
force-pushed
the
refactor_agregations
branch
from
9bd8fc5 to
923bc25
Compare
Signed-off-by: Nghia Truong <[email protected]> # Conflicts: # cpp/include/cudf/detail/aggregation/aggregation.hpp
…derived classes Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
ttnghia
commented
Jan 19, 2026
Comment on lines
+124
to
+135
| /** | ||
| * @brief No-parameter constructor. | ||
| * | ||
| * This constructor is never called anywhere, and should never be called at all. However, it | ||
| * cannot be be declared as deleted due to the usage of CRTP (Curiously Recurring Template | ||
| * Pattern) helper to automatically implement `clone()` method for derived aggregation classes. As | ||
| * such, definition of this constructor is just to satisfy the compiler requirements. | ||
| */ | ||
| aggregation() : kind{static_cast<Kind>(-1)} | ||
| { | ||
| CUDF_FAIL("No-parameter aggregation constructor should never be called"); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This was not an issue before, until this PR that removes all virtual function implementation from the most derived classes. Now, only the middle base class (derive_from) has virtual function implementation, causing the compiler to require a default constructor for it, which in turn requires to call into the default constructor of aggregation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the
simple_aggregations_collectorandaggregation_finalizervisitor-pattern classes and replaces them with template functors specialized only for aggregation types requiring special handling.Changes
simple_aggregations_collectorandaggregation_finalizerbase classesget_simple_aggregations()andfinalize()virtual methods from all aggregation classessimple_aggregation_collector_fn– decomposes compound aggregations (MEAN→SUM+COUNT, etc.)hash_compound_agg_finalizer_fn– postprocesses compound aggregation resultsrolling_preprocessor_fn/rolling_postprocessor_fn– rolling window aggregation operatorscudf::detail::aggregation_dispatcher()with these functorsBenefits
Closes #21059.