[C++] Add/Sub/Mul arithmetic kernels with overflow check

[asfimport]

Currently the output type of the Add function is identical with the argument types which makes it unsafe to add numeric limit values, so instead of using (int8, int8) -> int8 signature we should use ((int8, int8) -> int16.

The current kernels do wrap around for performance reasons, so implement different kernels with overflow checking and add arithmetics options to let the caller choose which variant to run

Reporter: Krisztian Szucs / @kszucs
Assignee: Krisztian Szucs / @kszucs

Related issues:

• [R] Add bindings for arithmetic kernels (is depended upon by)

PRs and other links:

• GitHub Pull Request #7341
• GitHub Pull Request #7420

_{Note: This issue was originally created as ARROW-9022. Please see the migration documentation for further details.}

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
For reference (not that this is necessarily an argument), numpy preserves the data types in addition (so int8 + int8 -> int8)

[asfimport]

Krisztian Szucs / @kszucs:
It's not trivial which behaviour to choose. We may introduce options for the arithmetic kernels to select the right one.

As a reference clickhouse follows the upcasting semantics, seems like Impala and DuckDB use the same types.

[asfimport]

Ben Kietzman / @bkietz:
Replicating #7341 (comment) here:

I'm not sure this approach is desirable. For one thing this leads to inconsistent handling of 64 bit integer types, which are currently allowed to overflow (NB: that means we would incur undefined behavior for int64).

There are a few other approaches we could take (ordered by personal preference):

• define explicit overflow behavior for signed integer operands (for example if we declared that add(i8(a), i8(b)) will always be equivalent to i8(i16(a) + i16(b)) then we could instantiate only unsigned addition kernels)
• raise an error on signed overflow
• provide ArithmeticOptions::overflow_behavior and allow users to choose between these
• require users to pass arguments which will not overflow

[asfimport]

Jacek Pliszka / @JacekPliszka:
I believe we should keep it identical. User can always promote/cast explicitly.

 

I am for Antoine's approach from mail thread - user selectable option - either error on overflow or ignore it.

[asfimport]

Wes McKinney / @wesm:
Issue resolved by pull request 7420
#7420