-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(cudf): Handle sliced decimal aggregation state #18608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,16 +98,18 @@ struct UnpackStateFunctor { | |
| const uint8_t* chars; | ||
| cuda::std::span<__int128_t> sums; | ||
| cuda::std::span<int64_t> counts; | ||
| cudf::size_type rowOffset; | ||
| cudf::bitmask_type const* nullMask; | ||
|
|
||
| __device__ void operator()(cudf::size_type idx) const { | ||
| if (nullMask && !cudf::bit_is_set(nullMask, idx)) { | ||
| auto const inputIdx = idx + rowOffset; | ||
| if (nullMask && !cudf::bit_is_set(nullMask, inputIdx)) { | ||
| return; | ||
| } | ||
| assert( | ||
| offsets[idx + 1] - offsets[idx] == | ||
| offsets[inputIdx + 1] - offsets[inputIdx] == | ||
| static_cast<OffsetT>(detail::kDecimalSumStateSize)); | ||
| int64_t offset = static_cast<int64_t>(offsets[idx]); | ||
| int64_t offset = static_cast<int64_t>(offsets[inputIdx]); | ||
| auto* state = reinterpret_cast<const DecimalSumState*>(chars + offset); | ||
| counts[idx] = state->count; | ||
| sums[idx] = (static_cast<__int128_t>(state->upper) << 64) | state->lower; | ||
|
|
@@ -239,22 +241,25 @@ struct unpackDecimalSumStateKernel { | |
| cudf::mutable_column_view sumView; | ||
| cudf::mutable_column_view countView; | ||
| cudf::size_type numRows; | ||
| cudf::size_type rowOffset; | ||
| cudf::bitmask_type const* nullMask; | ||
| rmm::cuda_stream_view stream; | ||
|
|
||
| template <typename OffsetT> | ||
| requires OffsetStorageType<OffsetT> | ||
| void operator()() const { | ||
| auto const n = static_cast<size_t>(numRows); | ||
| auto const inputSize = static_cast<size_t>(rowOffset) + n; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Should we add a defensive check here to verify that |
||
| launchDeviceFor( | ||
| numRows, | ||
| [&] { | ||
| return UnpackStateFunctor<OffsetT>{ | ||
| cuda::std::span<const OffsetT>{ | ||
| offsetsView.data<OffsetT>(), n + 1}, | ||
| offsetsView.data<OffsetT>(), inputSize + 1}, | ||
| chars, | ||
| cuda::std::span<__int128_t>{sumView.data<__int128_t>(), n}, | ||
| cuda::std::span<int64_t>{countView.data<int64_t>(), n}, | ||
| rowOffset, | ||
| nullMask}; | ||
| }, | ||
| stream); | ||
|
|
@@ -345,12 +350,20 @@ void unpackDecimalSumState( | |
| cudf::mutable_column_view sumView, | ||
| cudf::mutable_column_view countView, | ||
| cudf::size_type numRows, | ||
| cudf::size_type rowOffset, | ||
| cudf::bitmask_type const* nullMask, | ||
| rmm::cuda_stream_view stream) { | ||
| cudf::type_dispatcher( | ||
| cudf::data_type{offsetType}, | ||
| unpackDecimalSumStateKernel{ | ||
| offsetsView, chars, sumView, countView, numRows, nullMask, stream}); | ||
| offsetsView, | ||
| chars, | ||
| sumView, | ||
| countView, | ||
| numRows, | ||
| rowOffset, | ||
| nullMask, | ||
| stream}); | ||
| } | ||
|
|
||
| void averageRoundDecimalSum( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| #include "velox/type/DecimalUtil.h" | ||
|
|
||
| #include <cudf/column/column_factories.hpp> | ||
| #include <cudf/copying.hpp> | ||
| #include <cudf/null_mask.hpp> | ||
| #include <cudf/strings/strings_column_view.hpp> | ||
| #include <cudf/utilities/default_stream.hpp> | ||
|
|
@@ -1281,6 +1282,39 @@ TEST_F(CudfDecimalTest, decimalDeserializeSumStatePartialNullCompact) { | |
| EXPECT_EQ(outCount[2], 2); | ||
| } | ||
|
|
||
| TEST_F(CudfDecimalTest, decimalDeserializeSumStateSlice) { | ||
| auto stream = cudf::get_default_stream(); | ||
| auto mr = cudf::get_current_device_resource_ref(); | ||
|
|
||
| // Slice [1, 4) so the deserializer must apply a non-zero parent offset to | ||
| // both the strings offsets child and the parent validity mask. | ||
| std::vector<int64_t> sums = {10, 20, 0, 40, 50}; | ||
| std::vector<int64_t> counts = {1, 2, 0, 4, 5}; | ||
| std::vector<bool> sumValid = {true, true, false, true, true}; | ||
| auto sumCol = makeDecimalColumn<int64_t>(sums, 2, &sumValid, stream); | ||
| auto countCol = makeInt64Column(counts, nullptr, stream); | ||
| auto stateCol = | ||
| serializeDecimalSumState(sumCol->view(), countCol->view(), stream, mr); | ||
|
|
||
| auto slices = cudf::slice(stateCol->view(), {1, 4}); | ||
| ASSERT_EQ(slices.size(), 1); | ||
| ASSERT_EQ(slices.front().offset(), 1); | ||
|
|
||
| auto result = deserializeDecimalSumState(slices.front(), 2, stream); | ||
| auto outSum = copyColumnData<__int128_t>(result.sum->view(), stream); | ||
| auto outCount = copyColumnData<int64_t>(result.count->view(), stream); | ||
| auto outMask = copyNullMask(result.sum->view(), stream); | ||
|
|
||
| ASSERT_EQ(outSum.size(), 3); | ||
| EXPECT_TRUE(isValidAt(outMask, 0)); | ||
| EXPECT_FALSE(isValidAt(outMask, 1)); | ||
| EXPECT_TRUE(isValidAt(outMask, 2)); | ||
| EXPECT_EQ(outSum[0], static_cast<__int128_t>(20)); | ||
| EXPECT_EQ(outCount[0], 2); | ||
| EXPECT_EQ(outSum[2], static_cast<__int128_t>(40)); | ||
| EXPECT_EQ(outCount[2], 4); | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Can we also add a test/modify the existing test so it it accepts a sliced view of an arrow compacted column? We can then verify if the parent offsets are correctly applied to both the offsets child and the null mask. |
||
| // Trailing null: the offset for the last row equals chars_size, so the kernel | ||
| // would read 32 bytes past the buffer end without the null-mask guard. | ||
| TEST_F(CudfDecimalTest, decimalDeserializeSumStateTrailingNullCompact) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broader question: Does this assert actually propagate error to host?