From 01db2a3a53d29ee5d0f4cd47f870526ceebfbab7 Mon Sep 17 00:00:00 2001 From: Mithun RK Date: Mon, 13 Sep 2021 21:21:42 -0700 Subject: [PATCH 1/7] Explicitly disable grouping on LIST. Fixes #8905. Attempting groupby aggregations with LIST keys leads to silent failures and bad results. For instance, attempting hash-based groupby aggregations with LIST keys only fails on DEBUG builds, thus: ``` /home/myth/dev/cudf/2/cpp/include/cudf/table/row_operators.cuh:447: unsigned int cudf: :element_hasher_with_seed::operator()(cudf::column_device_view, signed in t) const [with T = cudf::list_view; void * = (void *)nullptr; hash_function = default_ha sh; __nv_bool has_nulls = false]: block: [0,0,0], thread: [0,0,0] Assertion `false && "Unsupported type in hash."` failed. ``` In RELEASE builds, a copy of the input LIST column is returned, causing each output row to be interpreted as its own group. This commit adds an explicit failure for unsupported LIST groupby keys. --- cpp/src/groupby/groupby.cu | 3 ++ cpp/tests/CMakeLists.txt | 1 + cpp/tests/groupby/lists_tests.cpp | 69 +++++++++++++++++++++++++++++ cpp/tests/groupby/structs_tests.cpp | 35 ++++++++++----- 4 files changed, 98 insertions(+), 10 deletions(-) create mode 100644 cpp/tests/groupby/lists_tests.cpp diff --git a/cpp/src/groupby/groupby.cu b/cpp/src/groupby/groupby.cu index 533f193d692..d03238e4752 100644 --- a/cpp/src/groupby/groupby.cu +++ b/cpp/src/groupby/groupby.cu @@ -55,6 +55,9 @@ groupby::groupby(table_view const& keys, _column_order{column_order}, _null_precedence{null_precedence} { + auto is_list = [](auto const& col) { return col.type().id() == cudf::type_id::LIST; }; + auto has_list = std::any_of(keys.begin(), keys.end(), is_list); + CUDF_EXPECTS(not has_list, "Groupby does not support LIST columns as keys."); } // Select hash vs. sort groupby implementation diff --git a/cpp/tests/CMakeLists.txt b/cpp/tests/CMakeLists.txt index d9553d463ab..03f7967cee0 100644 --- a/cpp/tests/CMakeLists.txt +++ b/cpp/tests/CMakeLists.txt @@ -62,6 +62,7 @@ ConfigureTest(GROUPBY_TEST groupby/count_tests.cpp groupby/groups_tests.cpp groupby/keys_tests.cpp + groupby/lists_tests.cpp groupby/m2_tests.cpp groupby/min_tests.cpp groupby/max_scan_tests.cpp diff --git a/cpp/tests/groupby/lists_tests.cpp b/cpp/tests/groupby/lists_tests.cpp new file mode 100644 index 00000000000..11b8ffa92b9 --- /dev/null +++ b/cpp/tests/groupby/lists_tests.cpp @@ -0,0 +1,69 @@ +/* + * Copyright (c) 2021, NVIDIA CORPORATION. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include +#include +#include + +#include + +namespace cudf { +namespace test { + +template +struct groupby_lists_test : public cudf::test::BaseFixture { +}; + +TYPED_TEST_SUITE(groupby_lists_test, cudf::test::FixedWidthTypes); + +namespace { +// Checking with a single aggregation, and aggregation column. +// This test is orthogonal to the aggregation type; it focuses on testing the grouping +// with LISTS keys. +auto sum_agg() { return cudf::make_sum_aggregation(); } + +void test_sort_based_sum_agg(column_view const& keys, column_view const& values) +{ + test_single_agg( + keys, values, keys, values, sum_agg(), force_use_sort_impl::YES, null_policy::INCLUDE); +} + +void test_hash_based_sum_agg(column_view const& keys, column_view const& values) +{ + test_single_agg( + keys, values, keys, values, sum_agg(), force_use_sort_impl::NO, null_policy::INCLUDE); +} + +} // namespace + +TYPED_TEST(groupby_lists_test, top_level_lists_are_unsupported) +{ + // Test that grouping on LISTS columns fails visibly. + + // clang-format off + auto keys = lists_column_wrapper { {1,1}, {2,2}, {3,3}, {1,1}, {2,2} }; + auto values = fixed_width_column_wrapper { 0, 1, 2, 3, 4 }; + // clang-format on + + EXPECT_THROW(test_sort_based_sum_agg(keys, values), cudf::logic_error); + EXPECT_THROW(test_hash_based_sum_agg(keys, values), cudf::logic_error); +} + +} // namespace test +} // namespace cudf diff --git a/cpp/tests/groupby/structs_tests.cpp b/cpp/tests/groupby/structs_tests.cpp index 00126a4a5a0..3715ba8d17b 100644 --- a/cpp/tests/groupby/structs_tests.cpp +++ b/cpp/tests/groupby/structs_tests.cpp @@ -22,8 +22,6 @@ #include #include -#include "cudf/aggregation.hpp" -#include "cudf/types.hpp" using namespace cudf::test::iterators; @@ -34,7 +32,7 @@ template struct groupby_structs_test : public cudf::test::BaseFixture { }; -TYPED_TEST_CASE(groupby_structs_test, cudf::test::FixedWidthTypes); +TYPED_TEST_SUITE(groupby_structs_test, cudf::test::FixedWidthTypes); using V = int32_t; // Type of Aggregation Column. using M0 = int32_t; // Type of STRUCT's first (i.e. 0th) member. @@ -79,27 +77,43 @@ void print_agg_results(column_view const& keys, column_view const& vals) } } -void test_sum_agg(column_view const& keys, - column_view const& values, - column_view const& expected_keys, - column_view const& expected_values) +void test_sort_based_sum_agg(column_view const& keys, + column_view const& values, + column_view const& expected_keys, + column_view const& expected_values) { test_single_agg(keys, values, expected_keys, expected_values, sum_agg(), - force_use_sort_impl::NO, + force_use_sort_impl::YES, null_policy::INCLUDE); +} + +void test_hash_based_sum_agg(column_view const& keys, + column_view const& values, + column_view const& expected_keys, + column_view const& expected_values) +{ test_single_agg(keys, values, expected_keys, expected_values, sum_agg(), - force_use_sort_impl::YES, + force_use_sort_impl::NO, null_policy::INCLUDE); } +void test_sum_agg(column_view const& keys, + column_view const& values, + column_view const& expected_keys, + column_view const& expected_values) +{ + test_sort_based_sum_agg(keys, values, expected_keys, expected_values); + test_hash_based_sum_agg(keys, values, expected_keys, expected_values); +} + } // namespace TYPED_TEST(groupby_structs_test, basic) @@ -312,7 +326,8 @@ TYPED_TEST(groupby_structs_test, lists_are_unsupported) // clang-format on auto keys = structs{{member_0, member_1}}; - EXPECT_THROW(test_sum_agg(keys, values, keys, values), cudf::logic_error); + EXPECT_THROW(test_sort_based_sum_agg(keys, values, keys, values), cudf::logic_error); + EXPECT_THROW(test_hash_based_sum_agg(keys, values, keys, values), cudf::logic_error); } } // namespace test From da091482ddb8e6c185d90c4e41abc0a865c84d3c Mon Sep 17 00:00:00 2001 From: Mithun RK Date: Tue, 14 Sep 2021 17:46:40 -0700 Subject: [PATCH 2/7] Row equality checks moved to row_equality_comparator. --- cpp/include/cudf/table/row_operators.cuh | 63 ++++++++++++++++++++++++ cpp/src/groupby/groupby.cu | 3 -- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/cpp/include/cudf/table/row_operators.cuh b/cpp/include/cudf/table/row_operators.cuh index d174222b2ff..d80f520c0eb 100644 --- a/cpp/include/cudf/table/row_operators.cuh +++ b/cpp/include/cudf/table/row_operators.cuh @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -222,6 +223,66 @@ class element_equality_comparator { bool nulls_are_equal; }; +namespace detail { + +/** + * @brief Helper functor to check if two types `LhsType` and `RhsType` are comparable for equality. + */ +template +struct equality_comparable_functor { + template + bool __device__ operator()() const + { + return cudf::is_equality_comparable(); + } +}; + +/** + * @brief Helper functor to check if a specified column `rhs_col` can be compared for equality + * with type `LhsType`. + */ +struct lhs_equality_comparable_functor { + template + bool __device__ operator()(column_device_view rhs_col) const + { + return cudf::type_dispatcher(rhs_col.type(), equality_comparable_functor{}); + } +}; + +/** + * @brief Checks if two columns have types that permit equality comparisons + * + * @param lhs Left column for equality comparisons + * @param rhs Right column for equality comparisons + * @return true If `lhs` and `rhs` columns have types whose values may be compared for equality + * @return false Otherwise. + */ +bool __device__ columns_are_equality_comparable(column_device_view lhs, column_device_view rhs) +{ + return cudf::type_dispatcher(lhs.type(), lhs_equality_comparable_functor{}, rhs); +} + +/** + * @brief Checks if corresponding columns in `lhs` and `rhs` tables may be compared for equality + * + * @param lhs Left table, whose columns are to be compared for equality. + * @param rhs Right table, whose columns are to be compared for equality. + * @return true If *all* columns in `lhs` permit equality comparisons with corresponding columns in + * `rhs`. + * @return false Otherwise. + */ +bool all_columns_are_equality_comparable(table_device_view lhs, table_device_view rhs) +{ + auto columns_begin = thrust::make_counting_iterator(0); + auto columns_end = columns_begin + lhs.num_columns(); + auto eq_comparable = [lhs, rhs] __device__(auto col_idx) { + return columns_are_equality_comparable(lhs.column(col_idx), rhs.column(col_idx)); + }; + return thrust::all_of(thrust::seq, columns_begin, columns_end, eq_comparable); +} + +} // namespace detail + template class row_equality_comparator { public: @@ -229,6 +290,8 @@ class row_equality_comparator { : lhs{lhs}, rhs{rhs}, nulls_are_equal{nulls_are_equal} { CUDF_EXPECTS(lhs.num_columns() == rhs.num_columns(), "Mismatched number of columns."); + CUDF_EXPECTS(all_columns_are_equality_comparable(lhs, rhs), + "Incompatible columns in row equality comparison."); } __device__ bool operator()(size_type lhs_row_index, size_type rhs_row_index) const noexcept diff --git a/cpp/src/groupby/groupby.cu b/cpp/src/groupby/groupby.cu index d03238e4752..533f193d692 100644 --- a/cpp/src/groupby/groupby.cu +++ b/cpp/src/groupby/groupby.cu @@ -55,9 +55,6 @@ groupby::groupby(table_view const& keys, _column_order{column_order}, _null_precedence{null_precedence} { - auto is_list = [](auto const& col) { return col.type().id() == cudf::type_id::LIST; }; - auto has_list = std::any_of(keys.begin(), keys.end(), is_list); - CUDF_EXPECTS(not has_list, "Groupby does not support LIST columns as keys."); } // Select hash vs. sort groupby implementation From 09ebe9996b5a94af32198c318b87339fe71d60cd Mon Sep 17 00:00:00 2001 From: Mithun RK Date: Wed, 15 Sep 2021 11:05:23 -0700 Subject: [PATCH 3/7] Moved equality_comparable checks back to groupby. --- cpp/include/cudf/table/row_operators.cuh | 63 ------------------------ cpp/src/groupby/groupby.cu | 19 +++++++ 2 files changed, 19 insertions(+), 63 deletions(-) diff --git a/cpp/include/cudf/table/row_operators.cuh b/cpp/include/cudf/table/row_operators.cuh index d80f520c0eb..d174222b2ff 100644 --- a/cpp/include/cudf/table/row_operators.cuh +++ b/cpp/include/cudf/table/row_operators.cuh @@ -25,7 +25,6 @@ #include #include -#include #include #include @@ -223,66 +222,6 @@ class element_equality_comparator { bool nulls_are_equal; }; -namespace detail { - -/** - * @brief Helper functor to check if two types `LhsType` and `RhsType` are comparable for equality. - */ -template -struct equality_comparable_functor { - template - bool __device__ operator()() const - { - return cudf::is_equality_comparable(); - } -}; - -/** - * @brief Helper functor to check if a specified column `rhs_col` can be compared for equality - * with type `LhsType`. - */ -struct lhs_equality_comparable_functor { - template - bool __device__ operator()(column_device_view rhs_col) const - { - return cudf::type_dispatcher(rhs_col.type(), equality_comparable_functor{}); - } -}; - -/** - * @brief Checks if two columns have types that permit equality comparisons - * - * @param lhs Left column for equality comparisons - * @param rhs Right column for equality comparisons - * @return true If `lhs` and `rhs` columns have types whose values may be compared for equality - * @return false Otherwise. - */ -bool __device__ columns_are_equality_comparable(column_device_view lhs, column_device_view rhs) -{ - return cudf::type_dispatcher(lhs.type(), lhs_equality_comparable_functor{}, rhs); -} - -/** - * @brief Checks if corresponding columns in `lhs` and `rhs` tables may be compared for equality - * - * @param lhs Left table, whose columns are to be compared for equality. - * @param rhs Right table, whose columns are to be compared for equality. - * @return true If *all* columns in `lhs` permit equality comparisons with corresponding columns in - * `rhs`. - * @return false Otherwise. - */ -bool all_columns_are_equality_comparable(table_device_view lhs, table_device_view rhs) -{ - auto columns_begin = thrust::make_counting_iterator(0); - auto columns_end = columns_begin + lhs.num_columns(); - auto eq_comparable = [lhs, rhs] __device__(auto col_idx) { - return columns_are_equality_comparable(lhs.column(col_idx), rhs.column(col_idx)); - }; - return thrust::all_of(thrust::seq, columns_begin, columns_end, eq_comparable); -} - -} // namespace detail - template class row_equality_comparator { public: @@ -290,8 +229,6 @@ class row_equality_comparator { : lhs{lhs}, rhs{rhs}, nulls_are_equal{nulls_are_equal} { CUDF_EXPECTS(lhs.num_columns() == rhs.num_columns(), "Mismatched number of columns."); - CUDF_EXPECTS(all_columns_are_equality_comparable(lhs, rhs), - "Incompatible columns in row equality comparison."); } __device__ bool operator()(size_type lhs_row_index, size_type rhs_row_index) const noexcept diff --git a/cpp/src/groupby/groupby.cu b/cpp/src/groupby/groupby.cu index 533f193d692..77c82bad4fe 100644 --- a/cpp/src/groupby/groupby.cu +++ b/cpp/src/groupby/groupby.cu @@ -43,6 +43,19 @@ namespace cudf { namespace groupby { +namespace { +/** + * @brief Helper functor to check if a specified type supports equality comparisons. + */ +struct equality_comparable_functor { + template + bool operator()() const + { + return cudf::is_equality_comparable(); + } +}; +} // namespace + // Constructor groupby::groupby(table_view const& keys, null_policy include_null_keys, @@ -55,6 +68,12 @@ groupby::groupby(table_view const& keys, _column_order{column_order}, _null_precedence{null_precedence} { + auto is_equality_comparable = [](auto const& col) { + return cudf::type_dispatcher(col.type(), equality_comparable_functor{}); + }; + auto all_keys_equality_comparable = std::all_of(keys.begin(), keys.end(), is_equality_comparable); + CUDF_EXPECTS(all_keys_equality_comparable, + "Unsupported groupby key type does not support equality comparison."); } // Select hash vs. sort groupby implementation From 321a19fc211a3d7f7a3b1c1483ae5b1706a2c6f2 Mon Sep 17 00:00:00 2001 From: Mithun RK Date: Wed, 15 Sep 2021 23:38:49 -0700 Subject: [PATCH 4/7] Added is_equality_comparable() trait. Also, added exceptions for STRING and STRUCT types as keys. --- cpp/include/cudf/utilities/traits.hpp | 25 +++++++++++++++++++++++++ cpp/src/groupby/groupby.cu | 25 +++++++------------------ 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/cpp/include/cudf/utilities/traits.hpp b/cpp/include/cudf/utilities/traits.hpp index f4e7e3e2a6d..40a833112e1 100644 --- a/cpp/include/cudf/utilities/traits.hpp +++ b/cpp/include/cudf/utilities/traits.hpp @@ -142,6 +142,31 @@ constexpr inline bool is_equality_comparable() return detail::is_equality_comparable_impl::value; } +namespace detail { +/** + * @brief Helper functor to check if a specified type `T` supports equality comparisons. + */ +struct unary_equality_comparable_functor { + template + bool operator()() const + { + return cudf::is_equality_comparable(); + } +}; +} // namespace detail + +/** + * @brief Checks whether `data_type` `type` supports equality comparisons. + * + * @param type Data_type for comparison. + * @return true If `type` supports equality comparisons. + * @return false If `type` does not support equality comparisons. + */ +inline bool is_equality_comparable(data_type type) +{ + return cudf::type_dispatcher(type, detail::unary_equality_comparable_functor{}); +} + /** * @brief Indicates whether the type `T` is a numeric type. * diff --git a/cpp/src/groupby/groupby.cu b/cpp/src/groupby/groupby.cu index 77c82bad4fe..f0f641db51f 100644 --- a/cpp/src/groupby/groupby.cu +++ b/cpp/src/groupby/groupby.cu @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -43,19 +44,6 @@ namespace cudf { namespace groupby { -namespace { -/** - * @brief Helper functor to check if a specified type supports equality comparisons. - */ -struct equality_comparable_functor { - template - bool operator()() const - { - return cudf::is_equality_comparable(); - } -}; -} // namespace - // Constructor groupby::groupby(table_view const& keys, null_policy include_null_keys, @@ -68,12 +56,13 @@ groupby::groupby(table_view const& keys, _column_order{column_order}, _null_precedence{null_precedence} { - auto is_equality_comparable = [](auto const& col) { - return cudf::type_dispatcher(col.type(), equality_comparable_functor{}); + auto is_supported_key_type = [](auto col) { + return col.type().id() == type_id::STRUCT // Struct does not support ==. + or col.type().id() == type_id::STRING // String does not support == on __host__. + or cudf::is_equality_comparable(col.type()); }; - auto all_keys_equality_comparable = std::all_of(keys.begin(), keys.end(), is_equality_comparable); - CUDF_EXPECTS(all_keys_equality_comparable, - "Unsupported groupby key type does not support equality comparison."); + CUDF_EXPECTS(std::all_of(keys.begin(), keys.end(), is_supported_key_type), + "Unsupported groupby key type"); } // Select hash vs. sort groupby implementation From f1f9893bfea3e4f66ffa5ef9a18143d4ead5a390 Mon Sep 17 00:00:00 2001 From: Mithun RK Date: Thu, 16 Sep 2021 16:20:33 -0700 Subject: [PATCH 5/7] Moved equality_comparable checks to after flattening: 1. No special handling for STRUCT, STRING. 2. Utility function for type checks. --- cpp/CMakeLists.txt | 1 + cpp/src/groupby/common/utils.cpp | 31 +++++++++++++++++++++++++++++ cpp/src/groupby/common/utils.hpp | 10 +++++++++- cpp/src/groupby/groupby.cu | 9 ++------- cpp/src/groupby/sort/sort_helper.cu | 2 ++ 5 files changed, 45 insertions(+), 8 deletions(-) create mode 100644 cpp/src/groupby/common/utils.cpp diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index de6530084ad..8992e13c483 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -213,6 +213,7 @@ add_library(cudf src/filling/repeat.cu src/filling/sequence.cu src/groupby/groupby.cu + src/groupby/common/utils.cpp src/groupby/hash/groupby.cu src/groupby/sort/aggregate.cpp src/groupby/sort/group_argmax.cu diff --git a/cpp/src/groupby/common/utils.cpp b/cpp/src/groupby/common/utils.cpp new file mode 100644 index 00000000000..bfc2d66e694 --- /dev/null +++ b/cpp/src/groupby/common/utils.cpp @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2021, NVIDIA CORPORATION. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include + +namespace cudf::groupby::detail { + +void assert_keys_equality_comparable(cudf::table_view const& keys) +{ + auto is_supported_key_type = [](auto col) { return cudf::is_equality_comparable(col.type()); }; + CUDF_EXPECTS(std::all_of(keys.begin(), keys.end(), is_supported_key_type), + "Unsupported groupby key type does not support equality comparison"); +} + +} // namespace cudf::groupby::detail diff --git a/cpp/src/groupby/common/utils.hpp b/cpp/src/groupby/common/utils.hpp index 3da20fb9af3..381dbfbd2d3 100644 --- a/cpp/src/groupby/common/utils.hpp +++ b/cpp/src/groupby/common/utils.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019-20, NVIDIA CORPORATION. + * Copyright (c) 2019-2021, NVIDIA CORPORATION. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -39,6 +39,14 @@ inline std::vector extract_results(host_span #include #include +#include #include #include @@ -56,13 +57,6 @@ groupby::groupby(table_view const& keys, _column_order{column_order}, _null_precedence{null_precedence} { - auto is_supported_key_type = [](auto col) { - return col.type().id() == type_id::STRUCT // Struct does not support ==. - or col.type().id() == type_id::STRING // String does not support == on __host__. - or cudf::is_equality_comparable(col.type()); - }; - CUDF_EXPECTS(std::all_of(keys.begin(), keys.end(), is_supported_key_type), - "Unsupported groupby key type"); } // Select hash vs. sort groupby implementation @@ -84,6 +78,7 @@ std::pair, std::vector> groupby::disp // Optionally flatten nested key columns. auto [flattened_keys, _, __, ___] = flatten_nested_columns(_keys, {}, {}, column_nullability::FORCE); + cudf::groupby::detail::assert_keys_equality_comparable(flattened_keys); auto [grouped_keys, results] = detail::hash::groupby(flattened_keys, requests, _include_null_keys, stream, mr); return std::make_pair(unflatten_nested_columns(std::move(grouped_keys), _keys), diff --git a/cpp/src/groupby/sort/sort_helper.cu b/cpp/src/groupby/sort/sort_helper.cu index 69d68f7b6bc..706fa847325 100644 --- a/cpp/src/groupby/sort/sort_helper.cu +++ b/cpp/src/groupby/sort/sort_helper.cu @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -102,6 +103,7 @@ sort_groupby_helper::sort_groupby_helper(table_view const& keys, auto [flattened_keys, _, __, struct_null_vectors] = flatten_nested_columns(keys, {}, {}, column_nullability::FORCE); + cudf::groupby::detail::assert_keys_equality_comparable(flattened_keys); _struct_null_vectors = std::move(struct_null_vectors); _keys = flattened_keys; From 783a8be19cc3980446bd5366f4df1b80d3ce1f28 Mon Sep 17 00:00:00 2001 From: Mithun RK Date: Thu, 16 Sep 2021 20:53:31 -0700 Subject: [PATCH 6/7] Move equality_comparable checks inline. --- cpp/CMakeLists.txt | 1 - cpp/src/groupby/common/utils.cpp | 31 ----------------------------- cpp/src/groupby/groupby.cu | 6 ++++-- cpp/src/groupby/sort/sort_helper.cu | 7 +++++-- 4 files changed, 9 insertions(+), 36 deletions(-) delete mode 100644 cpp/src/groupby/common/utils.cpp diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index 8992e13c483..de6530084ad 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -213,7 +213,6 @@ add_library(cudf src/filling/repeat.cu src/filling/sequence.cu src/groupby/groupby.cu - src/groupby/common/utils.cpp src/groupby/hash/groupby.cu src/groupby/sort/aggregate.cpp src/groupby/sort/group_argmax.cu diff --git a/cpp/src/groupby/common/utils.cpp b/cpp/src/groupby/common/utils.cpp deleted file mode 100644 index bfc2d66e694..00000000000 --- a/cpp/src/groupby/common/utils.cpp +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright (c) 2021, NVIDIA CORPORATION. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include - -#include -#include - -namespace cudf::groupby::detail { - -void assert_keys_equality_comparable(cudf::table_view const& keys) -{ - auto is_supported_key_type = [](auto col) { return cudf::is_equality_comparable(col.type()); }; - CUDF_EXPECTS(std::all_of(keys.begin(), keys.end(), is_supported_key_type), - "Unsupported groupby key type does not support equality comparison"); -} - -} // namespace cudf::groupby::detail diff --git a/cpp/src/groupby/groupby.cu b/cpp/src/groupby/groupby.cu index e620c06384e..bdaccba38dc 100644 --- a/cpp/src/groupby/groupby.cu +++ b/cpp/src/groupby/groupby.cu @@ -27,12 +27,12 @@ #include #include #include +#include #include #include #include #include #include -#include #include #include @@ -78,7 +78,9 @@ std::pair, std::vector> groupby::disp // Optionally flatten nested key columns. auto [flattened_keys, _, __, ___] = flatten_nested_columns(_keys, {}, {}, column_nullability::FORCE); - cudf::groupby::detail::assert_keys_equality_comparable(flattened_keys); + auto is_supported_key_type = [](auto col) { return cudf::is_equality_comparable(col.type()); }; + CUDF_EXPECTS(std::all_of(flattened_keys.begin(), flattened_keys.end(), is_supported_key_type), + "Unsupported groupby key type does not support equality comparison"); auto [grouped_keys, results] = detail::hash::groupby(flattened_keys, requests, _include_null_keys, stream, mr); return std::make_pair(unflatten_nested_columns(std::move(grouped_keys), _keys), diff --git a/cpp/src/groupby/sort/sort_helper.cu b/cpp/src/groupby/sort/sort_helper.cu index 706fa847325..c4905b86ab9 100644 --- a/cpp/src/groupby/sort/sort_helper.cu +++ b/cpp/src/groupby/sort/sort_helper.cu @@ -23,9 +23,10 @@ #include #include #include +#include #include #include -#include +#include #include #include @@ -103,7 +104,9 @@ sort_groupby_helper::sort_groupby_helper(table_view const& keys, auto [flattened_keys, _, __, struct_null_vectors] = flatten_nested_columns(keys, {}, {}, column_nullability::FORCE); - cudf::groupby::detail::assert_keys_equality_comparable(flattened_keys); + auto is_supported_key_type = [](auto col) { return cudf::is_equality_comparable(col.type()); }; + CUDF_EXPECTS(std::all_of(flattened_keys.begin(), flattened_keys.end(), is_supported_key_type), + "Unsupported groupby key type does not support equality comparison"); _struct_null_vectors = std::move(struct_null_vectors); _keys = flattened_keys; From 0dd72f760534f33bafbf37467d062abe959302d5 Mon Sep 17 00:00:00 2001 From: Mithun RK Date: Mon, 20 Sep 2021 09:31:07 -0700 Subject: [PATCH 7/7] Remove vestigial declarations in utils.hpp. --- cpp/src/groupby/common/utils.hpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cpp/src/groupby/common/utils.hpp b/cpp/src/groupby/common/utils.hpp index 381dbfbd2d3..2804dea576e 100644 --- a/cpp/src/groupby/common/utils.hpp +++ b/cpp/src/groupby/common/utils.hpp @@ -39,14 +39,6 @@ inline std::vector extract_results(host_span