Extend SUM_WITH_OVERFLOW reduction to signed integers and decimals - #22465
Conversation
PointKernel
added
feature request
|
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. |
|
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:
📝 WalkthroughWalkthroughExtends SUM_WITH_OVERFLOW from int64-only to signed integer and fixed-point types; implements a templated, type-dispatched reduction with an overflow-aware accumulator and struct result; updates docs and SPDX years; adds typed tests and error-case tests covering the new type matrix. ChangesSUM_WITH_OVERFLOW Reduction Type Extension
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/tests/reductions/reduction_tests.cpp (1)
3476-3707: ⚡ Quick winAdd sliced and multi-block cases to the new typed suite.
The implementation now runs through
column_device_viewiterators andtransform_reduce, but these tests only exercise small contiguous inputs. A sliced column and a> block_sizeinput would give much better coverage for the new offset-handling and parallel-reduction paths, especially for decimal inputs.As per coding guidelines,
cpp/**/*test*.{cu,cpp}: Test functions must cover edge cases: empty input, null values, sliced columns, boundary sizes, multi-block sizes.🤖 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 `@cpp/tests/reductions/reduction_tests.cpp` around lines 3476 - 3707, Add tests for sliced and multi-block inputs to the ReduceWithOverflowTest typed suite: create a sliced column (use cudf::test::column_wrapper's slice or cudf::slice to produce a column with non-zero offset) and run cudf::reduce(...) with *cudf::make_sum_with_overflow_aggregation<reduce_aggregation>() to verify correct sum and overflow extraction via ReduceWithOverflowTest::extract_sum_overflow, and add a large column whose size exceeds the backend block size (e.g., replicate a value to produce > block_size elements) to exercise the parallel reduction path; also add analogous cases for null-containing/sliced columns using make_null_col and for fixed_point types (numeric::decimal32/64/128) so the typed suite covers offset handling and multi-block reductions.
🤖 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/src/reductions/sum_with_overflow.cu`:
- Around line 129-133: The code currently treats an invalid init as if no init
was supplied; change the init handling in sum_overflow_result<DeviceType>
initial_value so that when init.has_value() is true you check
init.value().get().is_valid(stream) and if that check fails set
initial_value.valid = false (and do not seed initial_value.sum), otherwise seed
initial_value.sum from the scalar and set initial_value.valid = true; this makes
SUM_WITH_OVERFLOW propagate an invalid init to an invalid result consistent with
the other reductions (refer to symbols initial_value, sum_overflow_result,
init.value().get().is_valid).
- Around line 41-58: overflow_sum_op's operator() is not associative and
performs undefined signed overflow; fix by computing the combined sum in a safe,
well-defined wider or unsigned domain and deriving the overflow flag
deterministically so combination order doesn't matter: in
overflow_sum_op::operator(), when combining two sum_overflow_result<DeviceType>
values, first promote lhs.sum and rhs.sum to a wider integer type (or their
unsigned counterpart) or use a checked-add routine to compute a full-precision
result without invoking signed overflow, set overflow = lhs.overflow ||
rhs.overflow || (result out of DeviceType bounds), and store the
clamped/truncated DeviceType sum only after that safe check; also ensure that if
lhs.overflow or rhs.overflow is already true you do not perform a signed
addition on DeviceType (use the wider/unsigned accumulator or skip addition and
propagate a canonical value) so the combiner remains associative and avoids UB.
---
Nitpick comments:
In `@cpp/tests/reductions/reduction_tests.cpp`:
- Around line 3476-3707: Add tests for sliced and multi-block inputs to the
ReduceWithOverflowTest typed suite: create a sliced column (use
cudf::test::column_wrapper's slice or cudf::slice to produce a column with
non-zero offset) and run cudf::reduce(...) with
*cudf::make_sum_with_overflow_aggregation<reduce_aggregation>() to verify
correct sum and overflow extraction via
ReduceWithOverflowTest::extract_sum_overflow, and add a large column whose size
exceeds the backend block size (e.g., replicate a value to produce > block_size
elements) to exercise the parallel reduction path; also add analogous cases for
null-containing/sliced columns using make_null_col and for fixed_point types
(numeric::decimal32/64/128) so the typed suite covers offset handling and
multi-block reductions.
🪄 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: 71daa890-cfd2-48af-ba12-342ac80a5967
📒 Files selected for processing (3)
cpp/src/reductions/reductions.cppcpp/src/reductions/sum_with_overflow.cucpp/tests/reductions/reduction_tests.cpp
81caf9e to
f316ba0
Compare
shrshi
left a comment
shrshi
left a comment
There was a problem hiding this comment.
Minor nits, but looks great overall!
Note (courtesy Claude): can we also update the doxygen documentation for sum_with_overflow in reduction_functions.hpp to indicate that new types are supported?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/include/cudf/reduction.hpp`:
- Around line 80-82: The documentation for the throw condition in
cudf::reduction (the comment describing std::invalid_argument for the
sum_with_overflow reduction) uses the wrong parameter name `output_dtype`;
update that identifier to the correct API parameter name `output_type` so the
doc matches the function signature and references `sum_with_overflow` and
`output_type` consistently.
🪄 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: 2b6d5159-8e25-4479-b1ca-a98941211889
📒 Files selected for processing (3)
cpp/include/cudf/reduction.hppcpp/include/cudf/reduction/detail/reduction_functions.hppcpp/src/reductions/sum_with_overflow.cu
✅ Files skipped from review due to trivial changes (1)
- cpp/include/cudf/reduction/detail/reduction_functions.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/reductions/sum_with_overflow.cu
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
One comment (see above), otherwise LGTM.
… addr on sm_70/80/89)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/reductions/sum_with_overflow.cu (1)
103-107: ⚡ Quick winUse
cudf::get_current_device_resource_ref()for temporary allocations.The
overflow_scalarandchildrencolumns are temporaries—only used to construct the returnedstruct_scalar. Per coding guidelines, temporary memory should usecudf::get_current_device_resource_ref(), reserving the passed-inmrfor the final returned memory.♻️ Proposed fix
- auto overflow_scalar = cudf::make_fixed_width_scalar<bool>(overflow_value, stream, mr); + auto overflow_scalar = cudf::make_fixed_width_scalar<bool>(overflow_value, stream, cudf::get_current_device_resource_ref()); std::vector<std::unique_ptr<cudf::column>> children; - children.push_back(cudf::make_column_from_scalar(*sum_scalar, 1, stream, mr)); - children.push_back(cudf::make_column_from_scalar(*overflow_scalar, 1, stream, mr)); + children.push_back(cudf::make_column_from_scalar(*sum_scalar, 1, stream, cudf::get_current_device_resource_ref())); + children.push_back(cudf::make_column_from_scalar(*overflow_scalar, 1, stream, cudf::get_current_device_resource_ref()));As per coding guidelines: "Allocate temporary memory using cudf::get_current_device_resource_ref(), not the passed-in MR parameter".
🤖 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 `@cpp/src/reductions/sum_with_overflow.cu` around lines 103 - 107, The temporaries overflow_scalar and the temporary columns vector children (created via cudf::make_fixed_width_scalar and cudf::make_column_from_scalar around sum_scalar and overflow_scalar) should be allocated with cudf::get_current_device_resource_ref() instead of the passed-in mr; change the allocator argument for these temporary allocations to cudf::get_current_device_resource_ref() while still using the original mr for the final struct_scalar return so only final output uses the caller-provided memory resource.
🤖 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.
Nitpick comments:
In `@cpp/src/reductions/sum_with_overflow.cu`:
- Around line 103-107: The temporaries overflow_scalar and the temporary columns
vector children (created via cudf::make_fixed_width_scalar and
cudf::make_column_from_scalar around sum_scalar and overflow_scalar) should be
allocated with cudf::get_current_device_resource_ref() instead of the passed-in
mr; change the allocator argument for these temporary allocations to
cudf::get_current_device_resource_ref() while still using the original mr for
the final struct_scalar return so only final output uses the caller-provided
memory resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44779d2f-6a6b-4b4d-bc97-d4a86ddc6c3a
📒 Files selected for processing (1)
cpp/src/reductions/sum_with_overflow.cu
|
@mythrocks requesting your review as you just updated the related java code. |
|
Apologies for the delay. (A lot of reviews at once.) |
| assertEquals(DType.INT64, children[0].getType()); | ||
| assertEquals(DType.BOOL8, children[1].getType()); |
There was a problem hiding this comment.
This part of ReductionTest needs to change, to support all the new types we're adding.
At the time this was added, only INT64 reductions were possible. With added support for INT8-64, DECIMAL32-128, we can't assume that the result is INT64. :/
We will need a testSumWithOverflowSignedIntegralTypes and a testSumWithOverflowSignedDecimalTypes, for the positive cases. Those will call this helper function, to check for overflow.
There was a problem hiding this comment.
Additionally, we're going to need to update the Java documentation in:
- Aggregation.java: Indicate that
SUM_WITH_OVERFLOWaccepts INT8-64, DECIMAL32-128. - ReductionAggregation.java: Similar. Indicate that the output column is a
STRUCTof{appropriate_result_type, overflow: BOOL}.
There was a problem hiding this comment.
cc @paul-aiyedun, for Java expertise.
I'm wondering if it makes more sense to update the Java side of this change in a separate follow-on PR.
There was a problem hiding this comment.
@PointKernel: Would you mind raising a follow-up PR for the Java changes I suggested here? I don't mind that we postpone the changes. But it would be good for us to keep track.
There was a problem hiding this comment.
I should have said follow-up issue, not PR.
It would be good for this not to drift too long, given that there are other projects now dependent on cudf-java besides spark-rapids-jni.
There was a problem hiding this comment.
Thanks, opened #22642 to track the follow up work.
| @Test | ||
| void testSumWithOverflowPositiveOverflow() { | ||
| // Long.MAX_VALUE + 1 wraps via two's complement to Long.MIN_VALUE. | ||
| // Sum is zeroed when overflow is detected; the flag is the source of truth. |
There was a problem hiding this comment.
This is certainly the preferred behaviour, but I'm not sure that's what's happening in sum_with_overflow.cu. From there:
return make_sum_overflow_struct_scalar<Source>(
result.sum, result.wraps != 0, true, col.type(), stream, mr);This looks like we're returning the sum as is.
In sum_with_overflow.cu, should we instead consider checking wraps and returning 0 conditionally?
There was a problem hiding this comment.
(It's possible I've misunderstood this.)
There was a problem hiding this comment.
This is nice with reduce but introduced some unnecessary cycles for the groupby case (see #22696) where we need to either store zero atomically or require a second pass to zero all overflowed output. With that, I'm plan to update the default behavior of both reduce and groupby to provide undeterministic sum output by bypassing the sum action whenever we see the overflow flag is set. Users should never rely on the output sum once the overflow flag is set anyway. @mythrocks, does that make sense to you?
There was a problem hiding this comment.
Actually, yes. I think that should be good. We shouldn't really depend on the 0, if the overflow flag is set.
I brought it up since comment in the reduction-test indicates a zero sum. It sounded like a good idea, but maybe it's more trouble than it's worth.
Uncovered this cleanup opportunity while working on #22465. This PR replaces cudf's custom overflow handling logic with the overflow utilities provided by CCCL. Note that although these utilities are under the `cuda::` namespace, they are available on both host and device. Authors: - Yunsong Wang (https://github.com/PointKernel) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Muhammad Haseeb (https://github.com/mhaseeb123) - David Wendt (https://github.com/davidwendt) URL: #22593
mythrocks
left a comment
mythrocks
left a comment
There was a problem hiding this comment.
I'm 👍 on the change, with the proviso that we file a follow-up issue to address the remaining Java concerns described here:
We need to update ReductionTest.java with the newly supported types for SUM_WITH_OVERFLOW.
It looks like you've already updated the documentation for the Aggregation to indicate the newly supported types, and the behaviour in the event of an overflow. Thank you for that.
|
/merge |
Originally proposed by @shrshi at #19403 (comment) and follows the subsequent discussions in #22465 (comment) This PR updates both `groupby` and `reduce` `SUM_WITH_OVERFLOW` so that once a group's (or the reduction's) overflow flag is set, the sum is no longer maintained: `groupby` short-circuits subsequent atomic adds for that group, and `reduce` stops zeroing the wrapped result. The sum child is documented as unspecified on overflow; the boolean flag is the sole source of truth. Note that this is an ABI-breaking change, since the expected overflowed sum value is no longer the wrapped result. However, this should not impact downstream users, as relying on overflowed sum results is not considered valid behavior. Authors: - Yunsong Wang (https://github.com/PointKernel) Approvers: - Nghia Truong (https://github.com/ttnghia) - Vukasin Milovanovic (https://github.com/vuule) - David Wendt (https://github.com/davidwendt) URL: #22696
6 participants
Description
Closes #22413
This PR drops the INT64-only guard on the
SUM_WITH_OVERFLOWreduction and broadens it to all signed integer types (int8/int16/int32/int64) and decimal types (decimal32/64/128), matching the trait and the groupby path. Output preserves the input element type. Tests are unified into one typed fixture covering all newly supported types.Checklist