Skip to content

Fix scale handling of fixed-point types in minmax aggregation for cudf::reduce #23752

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.

Sign up for GitHub

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

Open
davidwendt wants to merge 6 commits into
base: main
Choose a base branch
from
+133 −7
Open

Fix scale handling of fixed-point types in minmax aggregation for cudf::reduce #23752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10b3280
Fix scale handling of fixed-point types in minmax aggregation for cud…
davidwendt Aug 20, 2026
0768d4d
add dictionary tests too
davidwendt Aug 20, 2026
8414070
Merge branch 'main' into fp-min-max
davidwendt Aug 20, 2026
6f30692
add is_valid checks to new gtests
davidwendt Aug 20, 2026
7273b53
Merge branch 'main' into fp-min-max
davidwendt Aug 21, 2026
8b67f62
fix var names
davidwendt Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions cpp/src/reductions/minmax.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <cudf/scalar/scalar_factories.hpp>
#include <cudf/utilities/default_stream.hpp>
#include <cudf/utilities/memory_resource.hpp>
#include <cudf/utilities/traits.hpp>

#include <cuda/iterator>
#include <cuda/std/functional>
Expand Down Expand Up @@ -144,6 +145,18 @@ struct assign_min_max {
T* max_data;
};

template <typename T>
std::unique_ptr<cudf::scalar_type_t<T>> make_minmax_scalar(cudf::data_type type,
cuda::stream_ref stream,
rmm::device_async_resource_ref mr)
{
if constexpr (cudf::is_fixed_point<T>()) {
return std::make_unique<cudf::scalar_type_t<T>>(
device_storage_type_t<T>{}, numeric::scale_type{type.scale()}, true, stream, mr);
}
return std::make_unique<cudf::scalar_type_t<T>>(T{}, true, stream, mr);
}

/**
* @brief Computes a minmax_pair<T> reduction directly over a dictionary column's decoded key
* values, i.e. `keys[indices[i]]` for each row `i`.
Expand Down Expand Up @@ -185,10 +198,9 @@ struct minmax_dictionary_functor {
{
using storage_type = device_storage_type_t<T>;
auto dev_result = reduce_dictionary<storage_type>(col, stream);
using ScalarType = cudf::scalar_type_t<T>;
auto const key_type = dictionary_column_view(col).keys().type();
auto minimum = std::make_unique<ScalarType>(T{}, true, stream, mr);
auto maximum = std::make_unique<ScalarType>(T{}, true, stream, mr);
auto minimum = make_minmax_scalar<T>(key_type, stream, mr);
auto maximum = make_minmax_scalar<T>(key_type, stream, mr);
cudf::detail::device_single_thread(
assign_min_max<storage_type>{dev_result.data(), minimum->data(), maximum->data()}, stream);
return {std::move(minimum), std::move(maximum)};
Expand Down Expand Up @@ -255,13 +267,12 @@ struct minmax_functor {
// compute minimum and maximum values
auto dev_result = reduce<storage_type>(col, stream);
// create output scalars
using ScalarType = cudf::scalar_type_t<T>;
auto minimum = new ScalarType(T{}, true, stream, mr);
auto maximum = new ScalarType(T{}, true, stream, mr);
auto minimum = make_minmax_scalar<T>(col.type(), stream, mr);
auto maximum = make_minmax_scalar<T>(col.type(), stream, mr);
// copy dev_result to the output scalars
cudf::detail::device_single_thread(
assign_min_max<storage_type>{dev_result.data(), minimum->data(), maximum->data()}, stream);
return {std::unique_ptr<scalar>(minimum), std::unique_ptr<scalar>(maximum)};
return {std::move(minimum), std::move(maximum)};
}

/**
Expand Down
115 changes: 115 additions & 0 deletions cpp/tests/reductions/reduction_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <cudf/copying.hpp>
#include <cudf/detail/iterator.cuh>
#include <cudf/dictionary/dictionary_column_view.hpp>
#include <cudf/dictionary/encode.hpp>
#include <cudf/dictionary/update_keys.hpp>
#include <cudf/fixed_point/fixed_point.hpp>
#include <cudf/reduction.hpp>
Expand Down Expand Up @@ -2179,6 +2180,61 @@ TYPED_TEST(FixedPointTestAllReps, FixedPointReductionMaxLarge)
}
}

TYPED_TEST(FixedPointTestAllReps, FixedPointMinMax)
{
using namespace numeric;
using decimalXX = TypeParam;
using RepType = cudf::device_storage_type_t<decimalXX>;
using fp_wrapper = cudf::test::fixed_point_column_wrapper<RepType>;

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<RepType>{1, scale}};
auto const expected_max = decimalXX{scaled_integer<RepType>{4, scale}};

auto const result = cudf::minmax(column);
auto const min_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(result.first.get());
auto const max_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(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<decimalXX>;
using fp_wrapper = cudf::test::fixed_point_column_wrapper<RepType>;

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<RepType>{1, scale}};
auto const expected_max = decimalXX{scaled_integer<RepType>{4, scale}};

auto const result = cudf::minmax(column);
auto const min_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(result.first.get());
auto const max_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(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;
Expand Down Expand Up @@ -3098,6 +3154,65 @@ TYPED_TEST(DictionaryReductionTest, Quantile)
45.0);
}

template <typename T>
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<decimalXX>;
using fp_wrapper = cudf::test::fixed_point_column_wrapper<RepType>;

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<RepType>{1, scale}};
auto const expected_max = decimalXX{scaled_integer<RepType>{4, scale}};

auto const result = cudf::minmax(dict->view());
auto const min_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(result.first.get());
auto const max_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(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);
EXPECT_TRUE(min_scalar->is_valid());
EXPECT_TRUE(max_scalar->is_valid());
}
}

TYPED_TEST(FixedPointDictionaryReductionTest, FixedPointDictionaryMinMaxWithNulls)
{
using namespace numeric;
using decimalXX = TypeParam;
using RepType = cudf::device_storage_type_t<decimalXX>;
using fp_wrapper = cudf::test::fixed_point_column_wrapper<RepType>;

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<RepType>{1, scale}};
auto const expected_max = decimalXX{scaled_integer<RepType>{5, scale}};

auto const result = cudf::minmax(dict->view());
auto const min_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(result.first.get());
auto const max_scalar = static_cast<cudf::scalar_type_t<decimalXX>*>(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);
Comment thread
davidwendt marked this conversation as resolved.
EXPECT_TRUE(min_scalar->is_valid());
EXPECT_TRUE(max_scalar->is_valid());
}
}

struct ListReductionTest : public cudf::test::BaseFixture {
void reduction_test(cudf::column_view const& input_data,
cudf::column_view const& expected_value,
Expand Down
Loading