diff --git a/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md b/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md index 8c7f97360123..d8758dcacbf5 100644 --- a/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md +++ b/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md @@ -1578,7 +1578,7 @@ the null masks of both struct fields. ## Dictionary columns Dictionaries provide an efficient way to represent low-cardinality data by storing a single copy -of each value. A dictionary comprises a column of distinct keys and a column containing an index into +of each value. A dictionary comprises a column of keys and a column containing an index into the keys column for each row of the parent column. The keys column may have any fixed-width data_type or STRING data_type. The indices represent the corresponding positions of each element's value in the keys. The indices child column can have any signed integer type @@ -1589,8 +1589,11 @@ input column will produce equivalent dictionary columns but the keys may be in a and therefore the indices will not match as well. Using `cudf::dictionary::decode()` on both dictionary columns should produce the same result. -Although `cudf::make_dictionary_column()` expects distinct keys, the API does not enforce this constraint. -Using a dictionary column with non-distinct keys in libcudf APIs may result in undefined behavior. +The libcudf APIs also accept dictionary columns with non-unique keys. +However, output dictionary columns will generally contain unique keys in an unspecified order. +The exceptions are `cudf::make_dictionary_column()`, which accepts keys and indices without +changing them, and `cudf::dictionary::set_keys()`, which strictly honors the given keys +(both order and duplicates). ## Nested column challenges diff --git a/cpp/include/cudf/dictionary/dictionary_column_view.hpp b/cpp/include/cudf/dictionary/dictionary_column_view.hpp index df48faee484b..43874219c266 100644 --- a/cpp/include/cudf/dictionary/dictionary_column_view.hpp +++ b/cpp/include/cudf/dictionary/dictionary_column_view.hpp @@ -22,7 +22,8 @@ namespace CUDF_EXPORT cudf { * @brief A wrapper class for operations on a dictionary column. * * A dictionary column contains a set of keys and a column of indices. - * The keys are a sorted set of unique values for the column. + * The keys are not required to be unique. + * * The indices represent the corresponding positions of each element's * value in the keys. */ diff --git a/cpp/include/cudf/dictionary/dictionary_factories.hpp b/cpp/include/cudf/dictionary/dictionary_factories.hpp index aa2fb92c7bf3..557d80958172 100644 --- a/cpp/include/cudf/dictionary/dictionary_factories.hpp +++ b/cpp/include/cudf/dictionary/dictionary_factories.hpp @@ -26,10 +26,7 @@ namespace CUDF_EXPORT cudf { * @brief Construct a dictionary column by copying the provided `keys` * and `indices`. * - * It is expected that `keys_column.has_nulls() == false`. - * It is assumed the elements in `keys_column` are unique and - * are in a strict, total order. Meaning, `keys_column[i]` is ordered before - * `keys_column[i+1]` for all `i in [0,n-1)` where `n` is the number of keys. + * The keys_column must contain no nulls. * * The indices values must be in the range [0,keys_column.size()). * @@ -49,7 +46,7 @@ namespace CUDF_EXPORT cudf { * @throw std::invalid_argument if keys_column contains nulls * @throw std::invalid_argument if indices_column type is not a signed integer * - * @param keys_column Column of unique, ordered values to use as the new dictionary column's keys. + * @param keys_column Column of non-null values to use as the new dictionary column's keys. * @param indices_column Indices to use for the new dictionary column. * @param stream CUDA stream used for device memory operations and kernel launches. * @param mr Device memory resource used to allocate the returned column's device memory. @@ -66,16 +63,12 @@ std::unique_ptr make_dictionary_column( * and indices columns. * * The keys_column and indices columns must contain no nulls. - * It is assumed the elements in `keys_column` are unique and - * are in a strict, total order. Meaning, `keys_column[i]` is ordered before - * `keys_column[i+1]` for all `i in [0,n-1)` where `n` is the number of keys. - * * The indices values must be in the range [0,keys_column.size()). * * @throw std::invalid_argument if keys_column or indices_column contains nulls * @throw std::invalid_argument if indices_column type is not a signed integer type * - * @param keys_column Column of unique, ordered values to use as the new dictionary column's keys. + * @param keys_column Column of non-null values to use as the new dictionary column's keys. * @param indices_column Indices to use for the new dictionary column. * @param null_mask Null mask for the output column. * @param null_count Number of nulls for the output column. @@ -101,7 +94,7 @@ std::unique_ptr make_dictionary_column(std::unique_ptr keys_colu * * @throw cudf::logic_error if keys_column contains nulls * - * @param keys_column Column of unique, ordered values to use as the new dictionary column's keys. + * @param keys_column Column of non-null values to use as the new dictionary column's keys. * @param indices_column Indices values and null-mask to use for the new dictionary column. * @param stream CUDA stream used for device memory operations and kernel launches. * @param mr Device memory resource used to allocate the returned column's device memory. diff --git a/cpp/include/cudf/dictionary/update_keys.hpp b/cpp/include/cudf/dictionary/update_keys.hpp index fe66c05d7a14..bd48ae902b52 100644 --- a/cpp/include/cudf/dictionary/update_keys.hpp +++ b/cpp/include/cudf/dictionary/update_keys.hpp @@ -121,6 +121,9 @@ std::unique_ptr remove_unused_keys( * The indices are updated to reflect the position values of the new keys. * Any indices pointing to removed keys sets those rows to null. * + * Although duplicate keys are allowed, indices in the returned dictionary may + * only reference one of the duplicates. + * * @code{.pseudo} * d1 = {keys=["a", "b", "c"], indices=[2, 0, 1, 2, 1]} * d2 = set_keys(existing_dict, ["b","c","d"]) diff --git a/cpp/include/cudf/reduction/detail/reduction_functions.hpp b/cpp/include/cudf/reduction/detail/reduction_functions.hpp index 6c6a3cf3d93c..6eda5d39a003 100644 --- a/cpp/include/cudf/reduction/detail/reduction_functions.hpp +++ b/cpp/include/cudf/reduction/detail/reduction_functions.hpp @@ -107,11 +107,14 @@ std::unique_ptr max(column_view const& col, * If all elements in input column are null, output scalar is null. * * @param col input column to compute reduction + * @param dispatch_type The type to dispatch on. For dictionary columns this must be the keys type; + * for all other columns it must equal `col.type()`. * @param stream CUDA stream used for device memory operations and kernel launches * @param mr Device memory resource used to allocate the returned scalar's device memory * @return Index of the minimum element as scalar of type `output_dtype` */ std::unique_ptr argmin(column_view const& col, + data_type dispatch_type, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr); @@ -121,11 +124,14 @@ std::unique_ptr argmin(column_view const& col, * If all elements in input column are null, output scalar is null. * * @param col input column to compute reduction + * @param dispatch_type The type to dispatch on. For dictionary columns this must be the keys type; + * for all other columns it must equal `col.type()`. * @param stream CUDA stream used for device memory operations and kernel launches * @param mr Device memory resource used to allocate the returned scalar's device memory * @return Index of the maximum element as scalar of type `output_dtype` */ std::unique_ptr argmax(column_view const& col, + data_type dispatch_type, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr); diff --git a/cpp/src/reductions/argmax.cu b/cpp/src/reductions/argmax.cu index f0950ea25fd9..07d8d5234d47 100644 --- a/cpp/src/reductions/argmax.cu +++ b/cpp/src/reductions/argmax.cu @@ -1,21 +1,17 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ #include "extrema_utils.cuh" -#include -#include - namespace cudf::reduction::detail { std::unique_ptr argmax(column_view const& input, + data_type dispatch_type, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) { - auto const dispatch_type = - is_dictionary(input.type()) ? dictionary_column_view(input).indices().type() : input.type(); return type_dispatcher( dispatch_type, simple::detail::arg_minmax_dispatcher{}, input, stream, mr); } diff --git a/cpp/src/reductions/argmin.cu b/cpp/src/reductions/argmin.cu index ce3adcaa60e0..1ccc8dc6254b 100644 --- a/cpp/src/reductions/argmin.cu +++ b/cpp/src/reductions/argmin.cu @@ -1,21 +1,17 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ #include "extrema_utils.cuh" -#include -#include - namespace cudf::reduction::detail { std::unique_ptr argmin(column_view const& input, + data_type dispatch_type, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) { - auto const dispatch_type = - is_dictionary(input.type()) ? dictionary_column_view(input).indices().type() : input.type(); return type_dispatcher( dispatch_type, simple::detail::arg_minmax_dispatcher{}, input, stream, mr); } diff --git a/cpp/src/reductions/extrema_utils.cuh b/cpp/src/reductions/extrema_utils.cuh index 19a635d55275..b85097d9dab7 100644 --- a/cpp/src/reductions/extrema_utils.cuh +++ b/cpp/src/reductions/extrema_utils.cuh @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -8,6 +8,7 @@ #include "nested_types_extrema_utils.cuh" #include +#include #include #include #include @@ -98,6 +99,9 @@ class arg_minmax_dispatcher { } // This function is used for types such as string, timestamp, fixed point, etc. + // `input` may be the dictionary column itself (rather than its keys or a decoded column): + // the lexicographic self_comparator compares `keys[indices[i]]` for dictionary columns + // directly, so no decoding is required and no assumption is made that the keys are sorted. template [[nodiscard]] size_type find_arg_minmax(column_view const& input, rmm::cuda_stream_view stream) const @@ -121,10 +125,26 @@ class arg_minmax_dispatcher { rmm::cuda_stream_view stream) const requires(cudf::is_numeric()) // integer + floating point numbers { + using Op = std::conditional_t; + // Dictionary keys are not guaranteed to be sorted, so the index of the min/max index does + // not identify the min/max key. Read `keys[indices[i]]` directly per row via a lazy + // iterator instead of decoding (and copying) the whole column. + if (is_dictionary(input.type())) { + auto const d_dict = column_device_view::create(input, stream); + if (input.has_nulls()) { + auto const transformer = + Op{}.template get_null_replacing_element_transformer(); + auto const p = + cudf::dictionary::detail::make_dictionary_pair_iterator(*d_dict, true); + auto const it = thrust::make_transform_iterator(p, transformer); + return find_extremum_idx(it, input.size(), stream); + } + auto const it = cudf::dictionary::detail::make_dictionary_iterator(*d_dict); + return find_extremum_idx(it, input.size(), stream); + } if (input.has_nulls()) { - using Op = std::conditional_t; auto const d_input = column_device_view::create(input, stream); auto const transformer = Op{}.template get_null_replacing_element_transformer(); auto const it = @@ -150,9 +170,7 @@ class arg_minmax_dispatcher { rmm::device_async_resource_ref mr) const requires(is_supported()) { - auto const& values = - is_dictionary(input.type()) ? dictionary_column_view(input).get_indices_annotated() : input; - auto const idx = find_arg_minmax(values, stream); + auto const idx = find_arg_minmax(input, stream); return make_fixed_width_scalar(idx, stream, mr); } diff --git a/cpp/src/reductions/minmax.cu b/cpp/src/reductions/minmax.cu index 1a03e01df946..f643ce6aa11a 100644 --- a/cpp/src/reductions/minmax.cu +++ b/cpp/src/reductions/minmax.cu @@ -1,16 +1,16 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ #include #include -#include #include #include #include #include #include +#include #include #include #include @@ -23,7 +23,6 @@ #include #include #include -#include #include #include @@ -129,6 +128,95 @@ struct create_minmax_with_nulls { } }; +/** + * @brief Functor to copy a minmax_pair result to individual scalar instances. + * + * @tparam T type of the data + * @tparam ResultType result type to assign min, max to minmax_pair + */ +template > +struct assign_min_max { + __device__ void operator()() + { + *min_data = result->min_val; + *max_data = result->max_val; + } + + ResultType* result; + T* min_data; + T* max_data; +}; + +/** + * @brief Computes a minmax_pair reduction directly over a dictionary column's decoded key + * values, i.e. `keys[indices[i]]` for each row `i`. + * + * @tparam T The dictionary's key type + */ +template +auto reduce_dictionary(column_view const& col, rmm::cuda_stream_view stream) +{ + auto d_dictionary = column_device_view::create(col, stream); + if (col.has_nulls()) { + auto pair_to_minmax = cuda::make_transform_iterator( + cudf::dictionary::detail::make_dictionary_pair_iterator(*d_dictionary, true), + create_minmax_with_nulls{}); + return reduce_device(pair_to_minmax, col.size(), minmax_binary_op{}, stream); + } else { + auto col_to_minmax = cuda::make_transform_iterator( + cudf::dictionary::detail::make_dictionary_iterator(*d_dictionary), create_minmax{}); + return reduce_device(col_to_minmax, col.size(), minmax_binary_op{}, stream); + } +} + +/** + * @brief Dispatch functor for minmax operation on a dictionary column, dispatched on the + * dictionary's key type. + */ +struct minmax_dictionary_functor { + template + static constexpr bool is_supported() + { + return !cudf::is_dictionary() && !std::is_same_v && + !std::is_same_v; + } + + template + std::pair, std::unique_ptr> operator()( + column_view const& col, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) + requires(is_supported() and !std::is_same_v) + { + 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); + cudf::detail::device_single_thread( + assign_min_max{dev_result.data(), minimum->data(), maximum->data()}, stream); + return {std::move(minimum), std::move(maximum)}; + } + + template + std::pair, std::unique_ptr> operator()( + column_view const& col, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) + requires(std::is_same_v) + { + auto dev_result = reduce_dictionary(col, stream); + auto const host_result = dev_result.value(stream); + return {std::make_unique(host_result.min_val, true, stream, mr), + std::make_unique(host_result.max_val, true, stream, mr)}; + } + + template + std::pair, std::unique_ptr> operator()( + column_view const&, rmm::cuda_stream_view, rmm::device_async_resource_ref) + requires(!is_supported()) + { + CUDF_FAIL("dictionary key type not supported for minmax() operation"); + } +}; + /** * @brief Dispatch functor for minmax operation. * @@ -150,35 +238,16 @@ struct minmax_functor { auto device_col = column_device_view::create(col, stream); // compute minimum and maximum values if (col.has_nulls()) { - auto pair_to_minmax = thrust::make_transform_iterator( - make_pair_iterator(*device_col), create_minmax_with_nulls{}); + auto pair_to_minmax = cuda::make_transform_iterator( + cudf::detail::make_pair_iterator(*device_col), create_minmax_with_nulls{}); return reduce_device(pair_to_minmax, col.size(), minmax_binary_op{}, stream); } else { auto col_to_minmax = - thrust::make_transform_iterator(device_col->begin(), create_minmax{}); + cuda::make_transform_iterator(device_col->begin(), create_minmax{}); return reduce_device(col_to_minmax, col.size(), minmax_binary_op{}, stream); } } - /** - * @brief Functor to copy a minmax_pair result to individual scalar instances. - * - * @tparam T type of the data - * @tparam ResultType result type to assign min, max to minmax_pair - */ - template > - struct assign_min_max { - __device__ void operator()() - { - *min_data = result->min_val; - *max_data = result->max_val; - } - - ResultType* result; - T* min_data; - T* max_data; - }; - template std::pair, std::unique_ptr> operator()( cudf::column_view const& col, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) @@ -223,15 +292,8 @@ struct minmax_functor { cudf::column_view const& col, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) requires(cudf::is_dictionary()) { - // computes minimum and maximum on the dictionary indices as dictionary32 values - auto d_indices = reduce(col, stream); - auto const indices = d_indices.value(stream); - // use these values to slice the keys column (add 1 for complete inclusion) - auto keys = cudf::detail::slice(dictionary_column_view(col).keys(), - {indices.min_val.value(), indices.max_val.value() + 1}, - stream) - .front(); - return type_dispatcher(keys.type(), minmax_functor{}, keys, stream, mr); + auto const keys_type = dictionary_column_view(col).keys().type(); + return type_dispatcher(keys_type, minmax_dictionary_functor{}, col, stream, mr); } template @@ -254,10 +316,13 @@ std::pair, std::unique_ptr> minmax( cudf::column_view const& col, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) { if (col.null_count() == col.size()) { - // this handles empty and all-null columns - // return scalars with valid==false - return {make_default_constructed_scalar(col.type(), stream, mr), - make_default_constructed_scalar(col.type(), stream, mr)}; + // this handles empty and all-null columns; return scalars with valid==false. + // For dictionary columns use the keys type — DICTIONARY32 has no scalar representation. + auto const scalar_type = col.type().id() == type_id::DICTIONARY32 + ? dictionary_column_view(col).keys().type() + : col.type(); + return {make_default_constructed_scalar(scalar_type, stream, mr), + make_default_constructed_scalar(scalar_type, stream, mr)}; } return type_dispatcher(col.type(), minmax_functor{}, col, stream, mr); diff --git a/cpp/src/reductions/reductions.cpp b/cpp/src/reductions/reductions.cpp index 0b2f63c997de..6c1d4d558503 100644 --- a/cpp/src/reductions/reductions.cpp +++ b/cpp/src/reductions/reductions.cpp @@ -140,7 +140,7 @@ struct reduction_function : public base_reduc CUDF_EXPECTS(params.output_dtype.id() == type_to_id(), "ARGMIN aggregation expects output type to be cudf::size_type", cudf::data_type_error); - return argmin(params.col, params.stream, params.mr); + return argmin(params.col, data_type{type_to_id()}, params.stream, params.mr); } }; @@ -151,7 +151,7 @@ struct reduction_function : public base_reduc CUDF_EXPECTS(params.output_dtype.id() == type_to_id(), "ARGMAX aggregation expects output type to be cudf::size_type", cudf::data_type_error); - return argmax(params.col, params.stream, params.mr); + return argmax(params.col, data_type{type_to_id()}, params.stream, params.mr); } }; diff --git a/cpp/tests/dictionary/add_keys_test.cpp b/cpp/tests/dictionary/add_keys_test.cpp index 38ac6c4caba8..3ea1d677d580 100644 --- a/cpp/tests/dictionary/add_keys_test.cpp +++ b/cpp/tests/dictionary/add_keys_test.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -58,6 +58,29 @@ TEST_F(DictionaryAddKeysTest, WithNull) CUDF_TEST_EXPECT_COLUMNS_EQUAL(decoded->view(), input); // new keys should not change anything } +TEST_F(DictionaryAddKeysTest, DuplicateKeys) +{ + auto input = cudf::test::dictionary_column_wrapper( + {"eee", "aaa", "ddd", "bbb", "ccc", "ccc", "ccc", "eee", "aaa"}); + + auto const dup_keys = cudf::test::strings_column_wrapper{"aaa", "ccc", "eee", "ccc"}; + auto const with_dups = cudf::dictionary::set_keys(input, dup_keys); // force duplicate keys + + // add_keys with a key already present + auto const added_keys = cudf::test::strings_column_wrapper{"ccc", "fff"}; + auto const result = cudf::dictionary::add_keys(with_dups->view(), added_keys); + auto const result_view = cudf::dictionary_column_view(result->view()); + + // old 4 keys + "fff" only — "ccc" must not have been added a third time + EXPECT_EQ(result_view.keys_size(), 5); + + // indices are unchanged so decode should match what set_keys produced + auto expected_decoded = cudf::test::strings_column_wrapper( + {"eee", "aaa", "", "", "ccc", "ccc", "ccc", "eee", "aaa"}, {1, 1, 0, 0, 1, 1, 1, 1, 1}); + auto const decoded = cudf::dictionary::decode(result_view); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(decoded->view(), expected_decoded); +} + TEST_F(DictionaryAddKeysTest, Errors) { cudf::test::fixed_width_column_wrapper input{1, 2, 3}; diff --git a/cpp/tests/dictionary/factories_test.cpp b/cpp/tests/dictionary/factories_test.cpp index fc0ff4042335..876bd3c74de4 100644 --- a/cpp/tests/dictionary/factories_test.cpp +++ b/cpp/tests/dictionary/factories_test.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -80,6 +80,24 @@ TEST_F(DictionaryFactoriesTest, ColumnsWithNulls) CUDF_TEST_EXPECT_COLUMNS_EQUAL(view.indices(), values_expected); } +TEST_F(DictionaryFactoriesTest, DuplicateKeys) +{ + std::vector h_keys{"pear", "apple", "fruit", "pear"}; + cudf::test::strings_column_wrapper keys(h_keys.begin(), h_keys.end()); + std::vector h_values{1, 2, 3, 1, 2, 3, 0}; + cudf::test::fixed_width_column_wrapper values(h_values.begin(), h_values.end()); + + auto dictionary = + cudf::make_dictionary_column(keys.release(), values.release(), rmm::device_buffer{}, 0); + cudf::dictionary_column_view view(dictionary->view()); + + auto keys_expected = cudf::test::strings_column_wrapper(h_keys.begin(), h_keys.end()); + auto values_expected = + cudf::test::fixed_width_column_wrapper(h_values.begin(), h_values.end()); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(view.keys(), keys_expected); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(view.indices(), values_expected); +} + TEST_F(DictionaryFactoriesTest, KeysWithNulls) { cudf::test::fixed_width_column_wrapper keys{{0, 1, 2, 3, 4}, diff --git a/cpp/tests/dictionary/gather_test.cpp b/cpp/tests/dictionary/gather_test.cpp index b8b22be3200f..763b495a29b2 100644 --- a/cpp/tests/dictionary/gather_test.cpp +++ b/cpp/tests/dictionary/gather_test.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -47,3 +48,59 @@ TEST_F(DictionaryGatherTest, GatherWithNulls) auto result_decoded = cudf::dictionary::decode(result); CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result_decoded->view()); } + +TEST_F(DictionaryGatherTest, DuplicateKeys) +{ + auto keys = cudf::test::strings_column_wrapper({"pear", "apple", "fruit", "pear"}); + auto values = cudf::test::fixed_width_column_wrapper({1, 2, 3, 1, 2, 3, 0}); + + auto dictionary = + cudf::make_dictionary_column(keys.release(), values.release(), rmm::device_buffer{}, 0); + auto view = cudf::dictionary_column_view(dictionary->view()); + + // decoded values are: {apple, fruit, pear, apple, fruit, pear, pear} + // where row 6 is the first 'pear' key and rows 2 and 5 are the second 'pear' key + { + // gather_map avoids any 'pear' rows + auto gather_map = cudf::test::fixed_width_column_wrapper({0, 4, 3, 1}); + auto table_result = cudf::gather(cudf::table_view{{view.parent()}}, gather_map)->release(); + auto result = cudf::dictionary_column_view(table_result.front()->view()); + + auto expected = cudf::test::strings_column_wrapper({"apple", "fruit", "apple", "fruit"}); + auto decoded = cudf::dictionary::decode(result); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, decoded->view()); + } + + { + // gather_map includes the first 'pear' row (index 6) + auto gather_map = cudf::test::fixed_width_column_wrapper({0, 6, 3, 1}); + auto table_result = cudf::gather(cudf::table_view{{view.parent()}}, gather_map)->release(); + auto result = cudf::dictionary_column_view(table_result.front()->view()); + + auto expected = cudf::test::strings_column_wrapper({"apple", "pear", "apple", "fruit"}); + auto decoded = cudf::dictionary::decode(result); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, decoded->view()); + } + + { + // gather_map includes the second 'pear' row (index 2) + auto gather_map = cudf::test::fixed_width_column_wrapper({0, 2, 4, 3}); + auto table_result = cudf::gather(cudf::table_view{{view.parent()}}, gather_map)->release(); + auto result = cudf::dictionary_column_view(table_result.front()->view()); + + auto expected = cudf::test::strings_column_wrapper({"apple", "pear", "fruit", "apple"}); + auto decoded = cudf::dictionary::decode(result); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, decoded->view()); + } + + { + // gather_map includes both the first and second 'pear' rows (indices 6 and 2) + auto gather_map = cudf::test::fixed_width_column_wrapper({6, 0, 2, 1}); + auto table_result = cudf::gather(cudf::table_view{{view.parent()}}, gather_map)->release(); + auto result = cudf::dictionary_column_view(table_result.front()->view()); + + auto expected = cudf::test::strings_column_wrapper({"pear", "apple", "pear", "fruit"}); + auto decoded = cudf::dictionary::decode(result); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, decoded->view()); + } +} diff --git a/cpp/tests/dictionary/remove_keys_test.cpp b/cpp/tests/dictionary/remove_keys_test.cpp index a249243f1ed2..412fcd419588 100644 --- a/cpp/tests/dictionary/remove_keys_test.cpp +++ b/cpp/tests/dictionary/remove_keys_test.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2020-2024, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -105,6 +105,54 @@ TEST_F(DictionaryRemoveKeysTest, WithNull) } } +TEST_F(DictionaryRemoveKeysTest, DuplicateKeys) +{ + auto input = cudf::test::dictionary_column_wrapper( + {"eee", "aaa", "ddd", "bbb", "ccc", "ccc", "ccc", "eee", "aaa"}); + + // set_keys with a duplicate "ccc" — position 3 is unreferenced + auto dup_keys = cudf::test::strings_column_wrapper{"aaa", "ccc", "eee", "ccc"}; + auto const with_dups = cudf::dictionary::set_keys(input, dup_keys); + + // remove "ccc" — both the indexed occurrence at position 1 and the unreferenced + // duplicate at position 3 must be dropped + auto del_keys = cudf::test::strings_column_wrapper{"ccc"}; + auto const result = + cudf::dictionary::remove_keys(cudf::dictionary_column_view(with_dups->view()), del_keys); + + EXPECT_EQ(cudf::dictionary_column_view(result->view()).keys_size(), 2); + + auto expected = cudf::test::strings_column_wrapper( + {"eee", "aaa", "", "", "", "", "", "eee", "aaa"}, {1, 1, 0, 0, 0, 0, 0, 1, 1}); + auto const decoded = cudf::dictionary::decode(result->view()); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(decoded->view(), expected); +} + +TEST_F(DictionaryRemoveKeysTest, RemoveUnusedAfterDuplicateSetKeys) +{ + auto input = cudf::test::dictionary_column_wrapper( + {"eee", "aaa", "ddd", "bbb", "ccc", "ccc", "ccc", "eee", "aaa"}); + + // force duplicate keys + auto new_keys = cudf::test::strings_column_wrapper{"aaa", "ccc", "eee", "ccc"}; + auto const with_dups = cudf::dictionary::set_keys(input, new_keys); + cudf::dictionary_column_view dups_view(with_dups->view()); + + // keys are stored in the order given, duplicate included + CUDF_TEST_EXPECT_COLUMNS_EQUAL(dups_view.keys(), new_keys); + + // remove_unused_keys should drop the unreferenced second "ccc" at position 3 + auto const cleaned = cudf::dictionary::remove_unused_keys(dups_view); + auto const cleaned_view = cudf::dictionary_column_view(cleaned->view()); + + EXPECT_EQ(cleaned_view.keys_size(), 3); + + auto expected_decoded = cudf::test::strings_column_wrapper( + {"eee", "aaa", "", "", "ccc", "ccc", "ccc", "eee", "aaa"}, {1, 1, 0, 0, 1, 1, 1, 1, 1}); + auto const decoded = cudf::dictionary::decode(cleaned_view); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(decoded->view(), expected_decoded); +} + TEST_F(DictionaryRemoveKeysTest, Errors) { cudf::test::fixed_width_column_wrapper input{1, 2, 3}; diff --git a/cpp/tests/dictionary/set_keys_test.cpp b/cpp/tests/dictionary/set_keys_test.cpp index 9a88ab9356c1..2e178c9fadac 100644 --- a/cpp/tests/dictionary/set_keys_test.cpp +++ b/cpp/tests/dictionary/set_keys_test.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -106,3 +106,19 @@ TEST_F(DictionarySetKeysTest, MatchDictionaries) auto expected2 = cudf::dictionary::decode(cudf::dictionary_column_view(col2)); CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(result2->view(), expected2->view()); } + +TEST_F(DictionarySetKeysTest, DuplicateKeys) +{ + auto input = cudf::test::strings_column_wrapper{ + "eee", "aaa", "ddd", "bbb", "ccc", "ccc", "ccc", "eee", "aaa"}; + auto dictionary = cudf::dictionary::encode(input); + + auto new_keys = cudf::test::strings_column_wrapper{"fff", "eee", "ccc", "aaa", "ccc"}; + auto result = cudf::dictionary::set_keys(dictionary->view(), new_keys); + EXPECT_TRUE(result->null_count() > 0); // duplicate keys should result in nulls + + auto expected = cudf::test::strings_column_wrapper( + {"eee", "aaa", "", "", "ccc", "ccc", "ccc", "eee", "aaa"}, {1, 1, 0, 0, 1, 1, 1, 1, 1}); + auto decoded = cudf::dictionary::decode(result->view()); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(*decoded, expected); +} diff --git a/cpp/tests/dictionary/sort_test.cpp b/cpp/tests/dictionary/sort_test.cpp index a6ef584fd5c3..66be19136931 100644 --- a/cpp/tests/dictionary/sort_test.cpp +++ b/cpp/tests/dictionary/sort_test.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -49,3 +50,21 @@ TEST_F(DictionarySortTest, SortFloat) auto expected = cudf::test::fixed_width_column_wrapper(h_input.begin(), h_input.end()); CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result_decoded->view()); } + +TEST_F(DictionarySortTest, DuplicateKeys) +{ + auto input = + cudf::test::dictionary_column_wrapper({"eee", "aaa", "ccc", "ccc", "eee", "aaa"}); + + // force duplicate keys + auto dup_keys = cudf::test::strings_column_wrapper{"aaa", "ccc", "eee", "ccc"}; + auto with_dups = cudf::dictionary::set_keys(input, dup_keys); + + auto result = cudf::sort(cudf::table_view{{with_dups->view()}}, + std::vector{cudf::order::ASCENDING}) + ->release(); + + auto result_decoded = cudf::dictionary::decode(result.front()->view()); + auto expected = cudf::test::strings_column_wrapper({"aaa", "aaa", "ccc", "ccc", "eee", "eee"}); + CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(expected, result_decoded->view()); +} diff --git a/cpp/tests/reductions/reduction_tests.cpp b/cpp/tests/reductions/reduction_tests.cpp index bd44f651de04..200dd98da132 100644 --- a/cpp/tests/reductions/reduction_tests.cpp +++ b/cpp/tests/reductions/reduction_tests.cpp @@ -12,6 +12,8 @@ #include #include +#include +#include #include #include #include @@ -2516,6 +2518,85 @@ TEST_P(DictionaryStringReductionTest, MinMax) output_type); } +TEST_P(DictionaryStringReductionTest, MinMaxUnsortedDuplicateKeys) +{ + std::vector host_strings(GetParam()); + cudf::data_type output_type{cudf::type_id::STRING}; + + cudf::test::dictionary_column_wrapper col(host_strings.begin(), host_strings.end()); + cudf::test::strings_column_wrapper new_keys{ + "nine", "two", "eight", "five", "six", "three", "two"}; + auto const dict = cudf::dictionary::set_keys(col, new_keys); + + auto const min_it = std::min_element(host_strings.begin(), host_strings.end()); + auto const max_it = std::max_element(host_strings.begin(), host_strings.end()); + + // MIN/MAX + this->reduction_test( + dict->view(), *min_it, true, *cudf::make_min_aggregation(), output_type); + this->reduction_test( + dict->view(), *max_it, true, *cudf::make_max_aggregation(), output_type); + + // ARGMIN/ARGMAX + this->reduction_test(dict->view(), + static_cast(std::distance(host_strings.begin(), min_it)), + *cudf::make_argmin_aggregation()); + this->reduction_test(dict->view(), + static_cast(std::distance(host_strings.begin(), max_it)), + *cudf::make_argmax_aggregation()); +} + +TEST_P(DictionaryStringReductionTest, MinMaxUnsortedDuplicateKeysWithNulls) +{ + // Rows 1 and 6 are null; "two" is the duplicate key (appears twice in new_keys). + // "two" would be the lexicographic max, so this verifies null rows are excluded. + // Valid rows: "nine"(0), "five"(2), "three"(3), "five"(4), "six"(5), "eight"(7), "nine"(8) + // => min="eight" at row 7, max="three" at row 3 + std::vector host_strings(GetParam()); + std::vector valid(host_strings.size(), true); + valid[1] = false; + valid[6] = false; + cudf::data_type output_type{cudf::type_id::STRING}; + + cudf::test::dictionary_column_wrapper col( + host_strings.begin(), host_strings.end(), valid.begin()); + cudf::test::strings_column_wrapper new_keys{ + "nine", "two", "eight", "five", "six", "three", "two"}; + auto const dict = cudf::dictionary::set_keys(col, new_keys); + + std::vector valid_strings; + for (size_t i = 0; i < host_strings.size(); ++i) { + if (valid[i]) { valid_strings.push_back(host_strings[i]); } + } + auto const expected_min = *std::min_element(valid_strings.begin(), valid_strings.end()); + auto const expected_max = *std::max_element(valid_strings.begin(), valid_strings.end()); + + this->reduction_test(dict->view(), + expected_min, + true, + *cudf::make_min_aggregation(), + output_type); + this->reduction_test(dict->view(), + expected_max, + true, + *cudf::make_max_aggregation(), + output_type); + + // argmin/argmax: expect the first valid row index that holds the min/max value + auto first_valid_idx = [&](std::string const& target) { + for (size_t i = 0; i < host_strings.size(); ++i) { + if (valid[i] && host_strings[i] == target) { return static_cast(i); } + } + return -1; + }; + this->reduction_test(dict->view(), + first_valid_idx(expected_min), + *cudf::make_argmin_aggregation()); + this->reduction_test(dict->view(), + first_valid_idx(expected_max), + *cudf::make_argmax_aggregation()); +} + template struct DictionaryAnyAllTest : public ReductionTest {}; using DictionaryAnyAllTypes = cudf::test::Types; @@ -2606,6 +2687,146 @@ TYPED_TEST(DictionaryReductionTest, Sum) expected_value); } +TYPED_TEST(DictionaryReductionTest, MinMaxUnsortedDuplicateKeys) +{ + using T = TypeParam; + std::vector int_values({50, 10, 40, 20, 30, 10, 50}); + std::vector v = convert_values(int_values); + cudf::data_type output_type{cudf::type_to_id()}; + + cudf::test::dictionary_column_wrapper col(v.begin(), v.end()); + std::vector key_values({30, 50, 10, 40, 20, 10}); + std::vector new_key_values = convert_values(key_values); + cudf::test::fixed_width_column_wrapper new_keys(new_key_values.begin(), new_key_values.end()); + auto const dict = cudf::dictionary::set_keys(col, new_keys); + + auto const min_it = std::min_element(v.begin(), v.end()); + auto const max_it = std::max_element(v.begin(), v.end()); + + EXPECT_EQ(this + ->template reduction_test( + dict->view(), *cudf::make_min_aggregation(), output_type) + .first, + *min_it); + EXPECT_EQ(this + ->template reduction_test( + dict->view(), *cudf::make_max_aggregation(), output_type) + .first, + *max_it); + + auto const res = cudf::minmax(dict->view()); + using ScalarType = cudf::scalar_type_t; + auto const min_result = static_cast(res.first.get()); + auto const max_result = static_cast(res.second.get()); + EXPECT_EQ(T{min_result->value()}, *min_it); + EXPECT_EQ(T{max_result->value()}, *max_it); + + EXPECT_EQ( + this + ->template reduction_test(dict->view(), + *cudf::make_argmin_aggregation(), + cudf::data_type{cudf::type_id::INT32}) + .first, + static_cast(std::distance(v.begin(), min_it))); + EXPECT_EQ( + this + ->template reduction_test(dict->view(), + *cudf::make_argmax_aggregation(), + cudf::data_type{cudf::type_id::INT32}) + .first, + static_cast(std::distance(v.begin(), max_it))); +} + +TYPED_TEST(DictionaryReductionTest, MinMaxAllNulls) +{ + using T = TypeParam; + std::vector int_values({5, 0, -14, 64}); + std::vector v = convert_values(int_values); + cudf::data_type output_type{cudf::type_to_id()}; + + std::vector all_null(v.size(), false); + cudf::test::dictionary_column_wrapper col(v.begin(), v.end(), all_null.begin()); + + // reduce min/max on an all-null dictionary must return an invalid (null) scalar + EXPECT_FALSE(this + ->template reduction_test( + col, *cudf::make_min_aggregation(), output_type) + .second); + EXPECT_FALSE(this + ->template reduction_test( + col, *cudf::make_max_aggregation(), output_type) + .second); + + // cudf::minmax must also return invalid scalars + auto const res = cudf::minmax(col); + using ScalarType = cudf::scalar_type_t; + auto const min_result = static_cast(res.first.get()); + auto const max_result = static_cast(res.second.get()); + EXPECT_FALSE(min_result->is_valid()); + EXPECT_FALSE(max_result->is_valid()); +} + +TYPED_TEST(DictionaryReductionTest, MinMaxUnsortedDuplicateKeysWithNulls) +{ + using T = TypeParam; + // Row 2 and row 5 are null; the duplicate key 10 appears at key positions 2 and 5. + // Null rows must be excluded from min/max even when their key value is extreme. + std::vector int_values({50, 10, 40, 20, 30, 10, 50}); + std::vector v = convert_values(int_values); + std::vector valid = {true, true, false, true, true, false, true}; + cudf::data_type output_type{cudf::type_to_id()}; + + cudf::test::dictionary_column_wrapper col(v.begin(), v.end(), valid.begin()); + std::vector key_values({30, 50, 10, 40, 20, 10}); + std::vector new_key_values = convert_values(key_values); + cudf::test::fixed_width_column_wrapper new_keys(new_key_values.begin(), new_key_values.end()); + auto const dict = cudf::dictionary::set_keys(col, new_keys); + + // Expected min/max considering only valid rows: {50, 10, -, 20, 30, -, 50} + std::vector valid_v; + for (size_t i = 0; i < v.size(); ++i) { + if (valid[i]) { valid_v.push_back(v[i]); } + } + auto const expected_min = *std::min_element(valid_v.begin(), valid_v.end()); + auto const expected_max = *std::max_element(valid_v.begin(), valid_v.end()); + + EXPECT_EQ(this + ->template reduction_test( + dict->view(), *cudf::make_min_aggregation(), output_type) + .first, + expected_min); + EXPECT_EQ(this + ->template reduction_test( + dict->view(), *cudf::make_max_aggregation(), output_type) + .first, + expected_max); + + auto const res = cudf::minmax(dict->view()); + using ScalarType = cudf::scalar_type_t; + auto const min_result = static_cast(res.first.get()); + auto const max_result = static_cast(res.second.get()); + EXPECT_EQ(T{min_result->value()}, expected_min); + EXPECT_EQ(T{max_result->value()}, expected_max); + + // argmin/argmax must also skip null rows + auto const argmin_idx = + this + ->template reduction_test(dict->view(), + *cudf::make_argmin_aggregation(), + cudf::data_type{cudf::type_id::INT32}) + .first; + auto const argmax_idx = + this + ->template reduction_test(dict->view(), + *cudf::make_argmax_aggregation(), + cudf::data_type{cudf::type_id::INT32}) + .first; + EXPECT_EQ(v[argmin_idx], expected_min); + EXPECT_EQ(v[argmax_idx], expected_max); + EXPECT_TRUE(valid[argmin_idx]); + EXPECT_TRUE(valid[argmax_idx]); +} + TYPED_TEST(DictionaryReductionTest, Product) { using T = TypeParam;