Skip to content

Commit

Permalink
revert sum agg (#14907)
Browse files Browse the repository at this point in the history
This pull request reverses the modifications made to the sum/product aggregation target type, ensuring it always produces int64. The changes implemented by  PR [14679](#14679) which led to degraded performance when the aggregation column had an unsigned type, are reverted. Additional details can be found in the issue [14886](#14886).

Authors:
   - Suraj Aralihalli (https://github.com/SurajAralihalli)

Approvers:
   - David Wendt (https://github.com/davidwendt)
   - Nghia Truong (https://github.com/ttnghia)
   - Karthikeyan (https://github.com/karthikeyann)
  • Loading branch information
SurajAralihalli authored Jan 29, 2024
1 parent 821f4de commit 7cd3834
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 9 deletions.
4 changes: 2 additions & 2 deletions cpp/include/cudf/detail/aggregation/aggregation.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1234,12 +1234,12 @@ constexpr bool is_sum_product_agg(aggregation::Kind k)
(k == aggregation::SUM_OF_SQUARES);
}

// Summing/Multiplying integers of any type, always use uint64_t for unsigned and int64_t for signed
// Summing/Multiplying integers of any type, always use int64_t accumulator
template <typename Source, aggregation::Kind k>
struct target_type_impl<Source,
k,
std::enable_if_t<std::is_integral_v<Source> && is_sum_product_agg(k)>> {
using type = std::conditional_t<std::is_unsigned_v<Source>, uint64_t, int64_t>;
using type = int64_t;
};

// Summing fixed_point numbers
Expand Down
11 changes: 4 additions & 7 deletions cpp/tests/groupby/sum_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ using namespace cudf::test::iterators;
template <typename V>
struct groupby_sum_test : public cudf::test::BaseFixture {};

using K = int32_t;
using supported_types = cudf::test::Concat<
cudf::test::Types<int8_t, int16_t, int32_t, int64_t, float, double, uint16_t, uint64_t>,
cudf::test::DurationTypes>;
using K = int32_t;
using supported_types =
cudf::test::Concat<cudf::test::Types<int8_t, int16_t, int32_t, int64_t, float, double>,
cudf::test::DurationTypes>;

TYPED_TEST_SUITE(groupby_sum_test, supported_types);

Expand All @@ -40,9 +40,6 @@ TYPED_TEST(groupby_sum_test, basic)
using V = TypeParam;
using R = cudf::detail::target_type_t<V, cudf::aggregation::SUM>;

static_assert(std::is_signed_v<R> == std::is_signed_v<V>,
"Both Result type and Source type must have same signedness");

cudf::test::fixed_width_column_wrapper<K> keys{1, 2, 3, 1, 2, 2, 1, 3, 3, 2};
cudf::test::fixed_width_column_wrapper<V> vals{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};

Expand Down

0 comments on commit 7cd3834

Please sign in to comment.