Skip to content

[FEA] [Java] Deprecate the use of Aggregation64Utils in favour of cudf::SUM_WITH_OVERFLOW #4632

Description

@mythrocks

Brief

It would be good to have Java users (eventually) switch to using the SUM_WITH_OVERFLOW cuDF aggregation, instead of using the workaround in Aggregation64Utils.java, for carrying out sum-operations on integral types and detecting overflows.

Background

  1. At the time Aggregation*Utils.java were written, libcudf didn't do overflow detection for SUM. When doing SUMs on DECIMAL128, only sort-based groupby aggregations were supported. The Spark RAPIDS plugin needed both.
  2. For use with the Spark RAPIDS, Aggregation64Utils.java was first written to address overflow detection when doing SUM(int64).
    • The trick was to split the 64-bit column into two 32-bit columns. Each column would produce a column of int64 values. The most significant 32 bits from the 64 bit sum of the lower half will be factored into the sum on the upper half.
  3. Similarly, Aggregation128Utils.java was written to solve the same problem similarly for Decimal128. It uses four sub-columns instead of two.
  4. As I understand it, in both cases, the sums on the intermediate columns cannot overflow since the intermediate results are int64, and each column can only have 2**31 rows.

What has changed now?

libcudf now supports SUM_WITH_OVERFLOW (hash) aggregations on integral types. This has some advantages:

  1. Aggregation64Utils is a bit of a workaround. We can get the same results as from Aggregation64Utils, but from a single kernel instead of three.
  2. Part of the reason the Aggregation64Utils doesn't overflow in its intermediate sums is that cudf::size_type == int32_t. It would be good not to have to depend on that. As I understand it, SUM_WITH_OVERFLOW ought to be able to catch the overflow as it happens.
  3. In the event that libcudf supports larger columns than can be represented in int32_t, it won't need addressing in Aggregation64Utils. :D

What are we proposing?

Once there are JNI bindings for cudf::SUM_WITH_OVERFLOW, I think it will be safe to migrate users of Aggregation64Utils to switch over to using SUM_WITH_OVERFLOW. I suspect it will be faster and safer.

What is out-of-scope?

While there is a lot of similarity with Aggregation64Utils, I don't propose that we change anything with Aggregation128Utils just yet (or at least, when we address this task):

  1. Aggregation128Utils is pulling double duty: overflow detection and supporting hash-aggregates. (It appears #20509 adds hash-agg support for SUM.)
  2. We might still need Aggregation128Utils for the reduction path. Note that cudf::reduce doesn't support SUM_WITH_OVERFLOW on DECIMAL128.

For this issue, perhaps we should keep the focus on Aggregation64Utils.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions