[BUG] Sum and multiply aggregations promote unsigned input types to a signed output

[jlowe]

Describe the bug When performing aggregations, the output types are often upscaled to help combat overflow situations. For example, performing a sum aggregation on an INT32 column will produce an INT64 result. However performing a sum aggregation on a UINT32 column produces an INT64 result rather than a UINT64 result.

Steps/Code to reproduce bug Perform a sum aggregation with an input column of UINT32 and note that the result is INT64. Here's a snippet of a session doing this with the cudf Java API in the Spark REPL shell:

scala> import ai.rapids.cudf._
import ai.rapids.cudf._

scala> val t = new Table(ColumnVector.fromInts(0), ColumnVector.fromUnsignedInts(0))
t: ai.rapids.cudf.Table = Table{columns=[ColumnVector{rows=1, type=INT32, nullCount=Optional[0], offHeap=(ID: 5 7feec1b5ac90)}, ColumnVector{rows=1, type=UINT32, nullCount=Optional[0], offHeap=(ID: 9 7feec1b5a280)}], cudfTable=140663428850592, rows=1}

scala> t.groupBy(0).aggregate(GroupByAggregation.sum().onColumn(1))
res0: ai.rapids.cudf.Table = Table{columns=[ColumnVector{rows=1, type=INT32, nullCount=Optional.empty, offHeap=(ID: 10 7ff0b7039860)}, ColumnVector{rows=1, type=INT64, nullCount=Optional.empty, offHeap=(ID: 11 7ff0b7039760)}], cudfTable=140671839344304, rows=1}

Expected behavior Unsigned input types should be promoted to unsigned output types for any aggregations where the sign of the result cannot change for unsigned inputs (e.g.: sum and multiply)

Additional context See @jrhemstad's comment at https://github.com/rapidsai/cudf/issues/10102#issuecomment-1023502832

[github-actions[bot]]

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

[github-actions[bot]]

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

[GregoryKimball]

From #10102 (comment):

I think the machinery in question was added before unsigned support and then was just never updated. It should be updated to use uint64 for unsigned integer types: https://github.com/rapidsai/cudf/blob/1246116c05abb6635624db25351f6596aa0abf2d/cpp/include/cudf/detail/aggregation/aggregation.hpp#L1140-L1147

could be updated to:

// 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<Source>::value && is_sum_product_agg(k)>> {
  using type = std::conditional_t<std::is_signed_v<Source>, int64_t, uint64_t>>;
};

[GregoryKimball]

The work in #14679 to address this issue ended up needed to be reverted in #14907 due to a performance regression reported in #14886.

In addition to adding back the changes in #14679, we also need to:

• add specializations in device_atomics.cuh for uint32_t and uint64_t as per this comment
• for clarity, update namespace for atomicAdd in aggregation.cuh to specify the cudf::detail:: namespace
• also for clarity, extend the cudf::detail:: namespace updates to atomicMul atomicMin atomicMax in aggregation.cuh

[bdice]

I started an experiment in this direction before I re-read this issue and realized @SurajAralihalli was assigned here. With apologies to @SurajAralihalli, I think I have a good start on the atomics refactoring in https://github.com/rapidsai/cudf/pull/14962. I would like to get that PR merged first, because it should be a standalone improvement, and then we can revisit the changes that were originally reverted.

[karthikeyann]

The revert can be undone after merging https://github.com/rapidsai/cudf/pull/14962. I tested a similar fix while debugging this issue with @SurajAralihalli . Although, a thorough testing on other types in similar scenario is required to ensure other bugs are not hidden. (perhaps test chrono types, decimal types).

[SurajAralihalli]

I started an experiment in this direction before I re-read this issue and realized @SurajAralihalli was assigned here. With apologies to @SurajAralihalli, I think I have a good start on the atomics refactoring in #14962. I would like to get that PR merged first, because it should be a standalone improvement, and then we can revisit the changes that were originally reverted.

Thanks @bdice for letting me know!