Rename sum_with_overflow aggregation to sum_overflow and deprecate old APIs - #23014
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test b747e87 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRenames the overflow-sum aggregation from ChangesSUM_WITH_OVERFLOW → SUM_OVERFLOW rename
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
java/src/test/java/ai/rapids/cudf/ReductionTest.java (1)
684-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one compatibility smoke test for the deprecated alias.
This suite now validates only
ReductionAggregation.sumOverflow(), but the PR’s compatibility promise is thatsumWithOverflow()still works. A single alias-based reduction test here would keep that guarantee from regressing silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/src/test/java/ai/rapids/cudf/ReductionTest.java` around lines 684 - 742, Add a compatibility smoke test in ReductionTest for the deprecated alias by exercising ReductionAggregation.sumWithOverflow() with the same kind of reduction path used in testSumOverflowNoOverflow. The new test should verify the alias still produces a valid SumOverflowResult with the expected sum and no overflow for a simple non-overflow input, so regressions in the alias are caught alongside the existing sumOverflow() coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/groupby/sum_overflow_tests.cpp`:
- Around line 25-35: Extend the groupby_sum_overflow_test suite by adding
SUM_OVERFLOW cases that use sliced input columns and inputs large enough to span
multiple blocks, since the current basic coverage only exercises empty/null and
hash-vs-sort paths. Add new tests alongside the existing
groupby_sum_overflow_test::basic coverage, using the same
sum_overflow_supported_types fixture, and verify both offset handling and
cross-block combine behavior for the renamed aggregation.
In `@java/src/main/java/ai/rapids/cudf/GroupByAggregation.java`:
- Around line 82-91: The Javadoc for GroupByAggregation.sumOverflow() still
describes outdated behavior and should be updated to match the current supported
sort-based groupby behavior exercised by TableTest. Edit the documentation on
the sumOverflow() factory in GroupByAggregation so it clearly states the actual
overflow-handling contract and no longer implies that sort-based groupby will
throw; keep the deprecated sumWithOverflow() alias pointing users to the updated
primary factory.
In `@java/src/test/java/ai/rapids/cudf/TableTest.java`:
- Around line 7929-7953: The two group-by overflow tests assume
`Table.groupBy(0).aggregate(...)` returns rows in key order, which is brittle.
Update `testGroupBySortSumOverflow` and
`testGroupBySortSumOverflowDetectsOverflow` to normalize the `results` table
with `orderBy(...)` on the group key before calling `assertSumOverflowResult`,
matching the pattern used by nearby aggregate tests. Keep the assertions
unchanged, but make sure the sorted output is what gets validated.
---
Nitpick comments:
In `@java/src/test/java/ai/rapids/cudf/ReductionTest.java`:
- Around line 684-742: Add a compatibility smoke test in ReductionTest for the
deprecated alias by exercising ReductionAggregation.sumWithOverflow() with the
same kind of reduction path used in testSumOverflowNoOverflow. The new test
should verify the alias still produces a valid SumOverflowResult with the
expected sum and no overflow for a simple non-overflow input, so regressions in
the alias are caught alongside the existing sumOverflow() coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 29982a88-5616-46d1-b169-65bc043fdb2b
📒 Files selected for processing (26)
cpp/CMakeLists.txtcpp/include/cudf/aggregation.hppcpp/include/cudf/detail/aggregation/aggregation.cuhcpp/include/cudf/detail/aggregation/aggregation.hppcpp/include/cudf/detail/aggregation/device_aggregators.cuhcpp/include/cudf/reduction.hppcpp/include/cudf/reduction/detail/reduction_functions.hppcpp/src/aggregation/aggregation.cppcpp/src/aggregation/aggregation.cucpp/src/groupby/common/utils.hppcpp/src/groupby/groupby.cucpp/src/groupby/hash/compute_single_pass_aggs.cucpp/src/groupby/hash/compute_single_pass_aggs.cuhcpp/src/groupby/hash/output_utils.cucpp/src/groupby/streaming_groupby/impl.cucpp/src/reductions/reductions.cppcpp/src/reductions/sum_overflow.cucpp/tests/CMakeLists.txtcpp/tests/groupby/sum_overflow_tests.cppcpp/tests/reductions/reduction_tests.cppjava/src/main/java/ai/rapids/cudf/Aggregation.javajava/src/main/java/ai/rapids/cudf/GroupByAggregation.javajava/src/main/java/ai/rapids/cudf/ReductionAggregation.javajava/src/main/native/src/AggregationJni.cppjava/src/test/java/ai/rapids/cudf/ReductionTest.javajava/src/test/java/ai/rapids/cudf/TableTest.java
|
/merge |
Description
Followup of #22832.
This PR completes the migration of the "sum with overflow" aggregation to the
_overflownaming so it is used consistently, and points the hash groupby and all remaining internal code at the canonicalSUM_OVERFLOWkind. #22832 addedSUM_OVERFLOW/make_sum_overflow_aggregation()for the sort path and leftSUM_WITH_OVERFLOW/make_sum_with_overflow_aggregation()as deprecated aliases; this PR migrates everything else.Checklist