From 10b3280819d44e0ab6ec2c0105e87a502b3bb44d Mon Sep 17 00:00:00 2001 From: David Wendt Date: Thu, 20 Aug 2026 17:23:18 -0400 Subject: [PATCH 1/4] Fix scale handling of fixed-point types in minmax aggregation for cudf::reduce --- cpp/src/reductions/minmax.cu | 27 +++++++++--- cpp/tests/reductions/reduction_tests.cpp | 55 ++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/cpp/src/reductions/minmax.cu b/cpp/src/reductions/minmax.cu index 5e58e0644ee7..1f45a1705fa3 100644 --- a/cpp/src/reductions/minmax.cu +++ b/cpp/src/reductions/minmax.cu @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -144,6 +145,20 @@ struct assign_min_max { T* max_data; }; +template +std::unique_ptr> make_minmax_scalar(cudf::data_type type, + cuda::stream_ref stream, + rmm::device_async_resource_ref mr) +{ + using ScalarType = cudf::scalar_type_t; + if constexpr (cudf::is_fixed_point()) { + using storage_type = device_storage_type_t; + return std::make_unique( + storage_type{}, numeric::scale_type{type.scale()}, true, stream, mr); + } + return std::make_unique(T{}, true, stream, mr); +} + /** * @brief Computes a minmax_pair reduction directly over a dictionary column's decoded key * values, i.e. `keys[indices[i]]` for each row `i`. @@ -185,10 +200,9 @@ struct minmax_dictionary_functor { { using storage_type = device_storage_type_t; auto dev_result = reduce_dictionary(col, stream); - using ScalarType = cudf::scalar_type_t; auto const key_type = dictionary_column_view(col).keys().type(); - auto minimum = std::make_unique(T{}, true, stream, mr); - auto maximum = std::make_unique(T{}, true, stream, mr); + auto minimum = make_minmax_scalar(key_type, stream, mr); + auto maximum = make_minmax_scalar(key_type, stream, mr); cudf::detail::device_single_thread( assign_min_max{dev_result.data(), minimum->data(), maximum->data()}, stream); return {std::move(minimum), std::move(maximum)}; @@ -255,13 +269,12 @@ struct minmax_functor { // compute minimum and maximum values auto dev_result = reduce(col, stream); // create output scalars - using ScalarType = cudf::scalar_type_t; - auto minimum = new ScalarType(T{}, true, stream, mr); - auto maximum = new ScalarType(T{}, true, stream, mr); + auto minimum = make_minmax_scalar(col.type(), stream, mr); + auto maximum = make_minmax_scalar(col.type(), stream, mr); // copy dev_result to the output scalars cudf::detail::device_single_thread( assign_min_max{dev_result.data(), minimum->data(), maximum->data()}, stream); - return {std::unique_ptr(minimum), std::unique_ptr(maximum)}; + return {std::move(minimum), std::move(maximum)}; } /** diff --git a/cpp/tests/reductions/reduction_tests.cpp b/cpp/tests/reductions/reduction_tests.cpp index 200dd98da132..571bf2548ef2 100644 --- a/cpp/tests/reductions/reduction_tests.cpp +++ b/cpp/tests/reductions/reduction_tests.cpp @@ -2179,6 +2179,61 @@ TYPED_TEST(FixedPointTestAllReps, FixedPointReductionMaxLarge) } } +TYPED_TEST(FixedPointTestAllReps, FixedPointMinMax) +{ + using namespace numeric; + using decimalXX = TypeParam; + using RepType = cudf::device_storage_type_t; + using fp_wrapper = cudf::test::fixed_point_column_wrapper; + + for (auto const i : {0, -1, -2, -3}) { + auto const scale = scale_type{i}; + auto const column = fp_wrapper{{2, 3, 1, 4}, scale}; + + auto const expected_min = decimalXX{scaled_integer{1, scale}}; + auto const expected_max = decimalXX{scaled_integer{4, scale}}; + + auto const result = cudf::minmax(column); + auto const min_scalar = static_cast*>(result.first.get()); + auto const max_scalar = static_cast*>(result.second.get()); + + // Scale must be preserved in the output scalars. + EXPECT_EQ(min_scalar->type().scale(), i); + EXPECT_EQ(max_scalar->type().scale(), i); + + EXPECT_EQ(min_scalar->fixed_point_value(), expected_min); + EXPECT_EQ(max_scalar->fixed_point_value(), expected_max); + } +} + +TYPED_TEST(FixedPointTestAllReps, FixedPointMinMaxWithNulls) +{ + using namespace numeric; + using decimalXX = TypeParam; + using RepType = cudf::device_storage_type_t; + using fp_wrapper = cudf::test::fixed_point_column_wrapper; + + for (auto const i : {0, -1, -2, -3}) { + auto const scale = scale_type{i}; + // valid: {2, null, 1, null, 4} — min=1, max=4 + auto const column = fp_wrapper{{2, 3, 1, 5, 4}, {true, false, true, false, true}, scale}; + + auto const expected_min = decimalXX{scaled_integer{1, scale}}; + auto const expected_max = decimalXX{scaled_integer{4, scale}}; + + auto const result = cudf::minmax(column); + auto const min_scalar = static_cast*>(result.first.get()); + auto const max_scalar = static_cast*>(result.second.get()); + + // Scale must be preserved in the output scalars. + EXPECT_EQ(min_scalar->type().scale(), i); + EXPECT_EQ(max_scalar->type().scale(), i); + + EXPECT_EQ(min_scalar->fixed_point_value(), expected_min); + EXPECT_EQ(max_scalar->fixed_point_value(), expected_max); + } +} + TYPED_TEST(FixedPointTestAllReps, FixedPointReductionNUnique) { using namespace numeric; From 0768d4d419645505ceb42e5537994704964e8562 Mon Sep 17 00:00:00 2001 From: David Wendt Date: Thu, 20 Aug 2026 17:31:37 -0400 Subject: [PATCH 2/4] add dictionary tests too --- cpp/tests/reductions/reduction_tests.cpp | 56 ++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/cpp/tests/reductions/reduction_tests.cpp b/cpp/tests/reductions/reduction_tests.cpp index 571bf2548ef2..30565e510590 100644 --- a/cpp/tests/reductions/reduction_tests.cpp +++ b/cpp/tests/reductions/reduction_tests.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -3153,6 +3154,61 @@ TYPED_TEST(DictionaryReductionTest, Quantile) 45.0); } +template +struct FixedPointDictionaryReductionTest : public cudf::test::BaseFixture {}; + +TYPED_TEST_SUITE(FixedPointDictionaryReductionTest, cudf::test::FixedPointTypes); + +TYPED_TEST(FixedPointDictionaryReductionTest, FixedPointDictionaryMinMax) +{ + using namespace numeric; + using decimalXX = TypeParam; + using RepType = cudf::device_storage_type_t; + using fp_wrapper = cudf::test::fixed_point_column_wrapper; + + for (auto const i : {0, -1, -2, -3}) { + auto const scale = scale_type{i}; + auto const col = fp_wrapper{{1, 2, 3, 4}, scale}; + auto const dict = cudf::dictionary::encode(col); + auto const expected_min = decimalXX{scaled_integer{1, scale}}; + auto const expected_max = decimalXX{scaled_integer{4, scale}}; + + auto const result = cudf::minmax(dict->view()); + auto const min_scalar = static_cast*>(result.first.get()); + auto const max_scalar = static_cast*>(result.second.get()); + + EXPECT_EQ(min_scalar->type().scale(), i); + EXPECT_EQ(max_scalar->type().scale(), i); + EXPECT_EQ(min_scalar->fixed_point_value(), expected_min); + EXPECT_EQ(max_scalar->fixed_point_value(), expected_max); + } +} + +TYPED_TEST(FixedPointDictionaryReductionTest, FixedPointDictionaryMinMaxWithNulls) +{ + using namespace numeric; + using decimalXX = TypeParam; + using RepType = cudf::device_storage_type_t; + using fp_wrapper = cudf::test::fixed_point_column_wrapper; + + for (auto const i : {0, -1, -2, -3}) { + auto const scale = scale_type{i}; + auto const col = fp_wrapper{{1, 2, 3, 4, 5}, {true, false, true, false, true}, scale}; + auto const dict = cudf::dictionary::encode(col); + auto const expected_min = decimalXX{scaled_integer{1, scale}}; + auto const expected_max = decimalXX{scaled_integer{5, scale}}; + + auto const result = cudf::minmax(dict->view()); + auto const min_scalar = static_cast*>(result.first.get()); + auto const max_scalar = static_cast*>(result.second.get()); + + EXPECT_EQ(min_scalar->type().scale(), i); + EXPECT_EQ(max_scalar->type().scale(), i); + EXPECT_EQ(min_scalar->fixed_point_value(), expected_min); + EXPECT_EQ(max_scalar->fixed_point_value(), expected_max); + } +} + struct ListReductionTest : public cudf::test::BaseFixture { void reduction_test(cudf::column_view const& input_data, cudf::column_view const& expected_value, From 6f30692ab2857f1bbd9a76e93816707bb31ea34a Mon Sep 17 00:00:00 2001 From: David Wendt Date: Thu, 20 Aug 2026 19:20:12 -0400 Subject: [PATCH 3/4] add is_valid checks to new gtests --- cpp/tests/reductions/reduction_tests.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cpp/tests/reductions/reduction_tests.cpp b/cpp/tests/reductions/reduction_tests.cpp index 30565e510590..67a388e41eca 100644 --- a/cpp/tests/reductions/reduction_tests.cpp +++ b/cpp/tests/reductions/reduction_tests.cpp @@ -3181,6 +3181,8 @@ TYPED_TEST(FixedPointDictionaryReductionTest, FixedPointDictionaryMinMax) EXPECT_EQ(max_scalar->type().scale(), i); EXPECT_EQ(min_scalar->fixed_point_value(), expected_min); EXPECT_EQ(max_scalar->fixed_point_value(), expected_max); + EXPECT_TRUE(min_scalar->is_valid()); + EXPECT_TRUE(max_scalar->is_valid()); } } @@ -3206,6 +3208,8 @@ TYPED_TEST(FixedPointDictionaryReductionTest, FixedPointDictionaryMinMaxWithNull EXPECT_EQ(max_scalar->type().scale(), i); EXPECT_EQ(min_scalar->fixed_point_value(), expected_min); EXPECT_EQ(max_scalar->fixed_point_value(), expected_max); + EXPECT_TRUE(min_scalar->is_valid()); + EXPECT_TRUE(max_scalar->is_valid()); } } From 8b67f62e60ec67ab7cd43f0dd0a39f6ad4f64a84 Mon Sep 17 00:00:00 2001 From: David Wendt Date: Fri, 21 Aug 2026 14:55:07 -0400 Subject: [PATCH 4/4] fix var names --- cpp/src/reductions/minmax.cu | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cpp/src/reductions/minmax.cu b/cpp/src/reductions/minmax.cu index 1f45a1705fa3..06264693d886 100644 --- a/cpp/src/reductions/minmax.cu +++ b/cpp/src/reductions/minmax.cu @@ -150,13 +150,11 @@ std::unique_ptr> make_minmax_scalar(cudf::data_type type, cuda::stream_ref stream, rmm::device_async_resource_ref mr) { - using ScalarType = cudf::scalar_type_t; if constexpr (cudf::is_fixed_point()) { - using storage_type = device_storage_type_t; - return std::make_unique( - storage_type{}, numeric::scale_type{type.scale()}, true, stream, mr); + return std::make_unique>( + device_storage_type_t{}, numeric::scale_type{type.scale()}, true, stream, mr); } - return std::make_unique(T{}, true, stream, mr); + return std::make_unique>(T{}, true, stream, mr); } /**