Change aggregation class hierarchy to allow per-algorithm type enforcement. #8052
Conversation
nvdbaranec
added
libcudf
|
Pinging @revans2. Let me know how you want to handle the timing on merging this once it gets approved. |
|
rerun tests |
I am working on a patch for the JNI code right now. Will post it when I am done. |
cpp/include/cudf/aggregation.hpp
Outdated
| virtual std::vector<aggregation::Kind> get_simple_aggregations(data_type col_type) const; | ||
| virtual void finalize(cudf::detail::aggregation_finalizer& finalizer); | ||
| protected: | ||
| rolling_aggregation(){}; |
There was a problem hiding this comment.
why is a ctor needed at all in this type?
There was a problem hiding this comment.
It gets implicitly called by all the derived classes. eg
class count_aggregation final : public rolling_aggregation {
public:
count_aggregation(aggregation::Kind kind) : aggregation(kind) {}
There was a problem hiding this comment.
Sure, but why isn't the compiler provided one sufficient?
|
#8069 is to get the java code working properly |
…ad of unique_ptrs.
…ter the finalizer. Refactor groupby to use this mechanism. Change finalizer to not be an abstract class in the interest of keeping it extensible for rolling window and other users.
nvdbaranec
added
4 - Needs cuDF (Python) Reviewer
and removed
5 - DO NOT MERGE
Codecov Report
@@ Coverage Diff @@
## branch-0.20 #8052 +/- ##
===============================================
- Coverage 82.88% 82.88% -0.01%
===============================================
Files 103 104 +1
Lines 17668 17894 +226
===============================================
+ Hits 14645 14832 +187
- Misses 3023 3062 +39
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Approving Python/Cython.
It's a shame that we'll have to carbon copy nearly everything once for every aggregation type as #7106 progresses. Since the set of possible aggregation types is known a priori, we should see if we can use Cython's fused types to more transparently wrap at least some of the templates. Templates and fused types don't play nearly as nicely together as I'd like, but we may at least be able to merge the cdef classes like RollingAggregation/Aggregation. That's out of scope for this PR though.
|
@gpucibot merge |
This depends on #8052 It adds in similar changes to the java APIs to have the compiler be able to check if an aggregation will work for rolling window or not. Authors: - Robert (Bobby) Evans (https://github.com/revans2) - https://github.com/nvdbaranec Approvers: - Jason Lowe (https://github.com/jlowe) URL: #8069
…e their usage. (#8906) Followup to #8052 Partially addresses #7106 Adds the `groupby_aggregation` class and forces usage of that type when calling `groupby::aggregate()` Adds the `groupby_scan_aggregation` class and forces usage of that type when calling `groupby::scan()` Authors: - https://github.com/nvdbaranec Approvers: - Robert (Bobby) Evans (https://github.com/revans2) - Jake Hemstad (https://github.com/jrhemstad) - David Wendt (https://github.com/davidwendt) - GALI PREM SAGAR (https://github.com/galipremsagar) - Nghia Truong (https://github.com/ttnghia) - Devavret Makkar (https://github.com/devavret) URL: #8906
vyasr
added
4 - Needs Review
Partially addresses #7106
Fundamentally, this changes the aggregation class hierarchy in the following ways:
aggregationclass becomes abstract, with theclone()andfinalize()functions being pure virtual.aggregation.rolling_aggregationare used to allow individual algorithms to only accept aggregation types that are valid for it (as opposed to enforcing this internally at runtime).All of the rolling_window interfaces have been updated to take a
rolling_aggregation. Other algorithms such as groupby are not yet converted and still take genericaggregationobjects.Marking this as Do Not Merge for now since this is a breaking change with immediately implications for Spark.