Add SUM_OVERFLOW in sort groupby - #22832
Conversation
Wire SUM_WITH_OVERFLOW into the sort groupby path via a new group_sum_with_overflow that reuses the reduction's overflow-aware sum operator, promoted to cudf/reduction/detail/sum_with_overflow.cuh. Previously the sort path threw for any co-requested sort-only aggregation such as TDIGEST (#22576). Adds sort-path tests across the supported signed integer and decimal types.
…rflow-sort-groupby
Deduplicate the "signed integral or decimal" predicate, previously copied across the reduction dispatcher, reduction_function, target_type, and the new sort groupby, into a single is_sum_with_overflow_supported helper in cudf/detail/aggregation/aggregation.hpp. The hash device aggregator keeps its own atomic-support-aware predicate. Also rename the null-replacement reduction functor to null_replaced_to_sum_overflow to match the existing null_replaced_value_accessor convention.
Bound both reduce_by_key calls in group_sum_with_overflow by group_labels.end() instead of values.size(), and trim a redundant supported-types comment on the target_type specialization.
…rflow-sort-groupby
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesOverflow-sum sort-path support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| thrust::reduce_by_key( | ||
| rmm::exec_policy_nosync(stream, cudf::get_current_device_resource_ref()), | ||
| group_labels.begin(), | ||
| group_labels.end(), |
There was a problem hiding this comment.
Can we combine two reduce_by_key together, using zip_iterator input/output? They are operating on the same keys.
There was a problem hiding this comment.
We need to do some benchmark to see how that combination will change performance.
There was a problem hiding this comment.
I noticed this also and realized that the sort-based regular SUM follows the same pattern. I originally tried using a single reduce_by_key invocation, but ran into issues that I mistakenly attributed to the single-pass approach. Now that we know the root cause is a bug elsewhere, let me take a quick look and see whether we can apply the same change to the regular SUM path as well.
There was a problem hiding this comment.
Benchmarked + profiled it: fusing the two reduce_by_key into one zip pass is consistently ~1.45x slower.
100M int64 rows, 90% valid, average of 50 iters:
| groups | two passes | fused one pass |
|---|---|---|
| 1 | 3.77 ms | 5.52 ms |
| 100 | 3.76 ms | 5.51 ms |
| 10K | 3.78 ms | 5.55 ms |
| 1M | 3.65 ms | 5.36 ms |
| 10M | 3.93 ms | 5.42 ms |
Root cause is register pressure: carrying tuple<{int64 sum, int32 wraps}, valid> through the CUB scan pushes registers from 71 to 89, which drops occupancy from 37% to 24% and slows the kernel. It's not bandwidth, the fused kernel actually reads fewer bytes. Keeping the two passes.
…rflow-sort-groupby
…um-with-overflow-sort-groupby
…rflow-sort-groupby
…groupby' into enable-sum-with-overflow-sort-groupby
…rflow-sort-groupby
…rflow-sort-groupby
…groupby' into enable-sum-with-overflow-sort-groupby
|
@ttnghia Can you please take another pass at this? I've made some additional Java changes since it was last approved. Thanks! |
|
/merge |
Closes #22576 This PR adds support for `SUM_OVERFLOW` (previously `SUM_WITH_OVERFLOW`) in the sort-based groupby execution path. Authors: - Yunsong Wang (https://github.com/PointKernel) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Nghia Truong (https://github.com/ttnghia) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #22832
…e old APIs (#23014) Followup of #22832. This PR completes the migration of the "sum with overflow" aggregation to the `_overflow` naming so it is used consistently, and points the hash groupby and all remaining internal code at the canonical `SUM_OVERFLOW` kind. #22832 added `SUM_OVERFLOW` / `make_sum_overflow_aggregation()` for the sort path and left `SUM_WITH_OVERFLOW` / `make_sum_with_overflow_aggregation()` as deprecated aliases; this PR migrates everything else. Authors: - Yunsong Wang (https://github.com/PointKernel) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #23014
Description
Closes #22576
This PR adds support for
SUM_OVERFLOW(previouslySUM_WITH_OVERFLOW) in the sort-based groupby execution path.Checklist