andygrove opened a new pull request, #5146:
URL: https://github.com/apache/datafusion-comet/pull/5146
## Which issue does this PR close?
Cherry-pick of #5084 (merged to `main` as `38c3f8bce`) onto `branch-1.0`.
Closes #5065 for the 1.0.0 release branch.
## Rationale for this change
Under ANSI mode, Spark throws `ARITHMETIC_OVERFLOW` ("Overflow in integral
divide") for `Long.MinValue div -1`, since the quotient `9223372036854775808`
cannot be represented as a `LONG`. Comet performs integral division by casting
both operands to `DECIMAL(19,0)`, dividing, and casting the result back to
`LONG` with legacy (wrapping) semantics, so it silently returned
`-9223372036854775808` instead of throwing.
Spark only performs this check when the operands are `LONG`
(`checkDivideOverflow` in `arithmetic.scala`): `DECIMAL` operands wrap around
on the implicit cast to `LONG` even in ANSI mode, and smaller integer types
cannot overflow because the result type is `LONG`. The fix therefore has to
know the original operand type, which is no longer visible on the native side
after the serde casts both operands to decimal.
This is a silent wrong-answer in ANSI mode on every supported Spark version,
so it is worth carrying into 1.0.0.
## What changes are included in this PR?
A clean `git cherry-pick -x` of `38c3f8bce`. No conflicts and no adaptation
was needed — the diff content is identical to the commit on `main` (only hunk
offsets differ, since `branch-1.0` does not carry every intervening `main`
commit).
- Adds a `check_divide_overflow` field to the `MathExpr` protobuf message,
set by `CometIntegralDivide` only when the left operand is `LONG` and eval mode
is ANSI (mirroring Spark's `checkDivideOverflow`). This is a new field on an
existing message, so it is wire-compatible within the release line.
- Plumbs the flag through `IntegralDivideBuilder` / `BinaryExprOptions` into
the existing `fail_on_error` parameter of
`create_comet_physical_fun_with_eval_mode`, with a new `make_comet_scalar_udf!`
arm that passes both `eval_mode` and `fail_on_error`.
- In `spark_decimal_div_internal`, when the flag is set and the quotient
does not fit in an `i64`, returns the previously never-constructed
`SparkError::IntegralDivideOverflow`, which already maps to
`SparkArithmeticException` with error class `ARITHMETIC_OVERFLOW`.
- Aligns the `IntegralDivideOverflow` message with Spark's wording (`Use
'try_divide' ...` with single quotes rather than backticks, matching Spark 3.4
through 4.1).
## How are these changes tested?
By the cases added to `expressions/math/arithmetic_ansi.sql` in #5084,
verified on this branch:
- `CometSqlFileTestSuite` filtered to `arithmetic_ansi` — passes. Covers `a
div b` and literal `div` for `Long.MinValue div -1` expecting
`ARITHMETIC_OVERFLOW` from both Spark and Comet, plus pinning queries for the
cases that must NOT throw: `INT_MIN div -1`, `LONG_MIN div 1`, and
`DECIMAL(19,0)` operands (which wrap around even in ANSI mode, matching Spark).
- `cargo clippy --all-targets --workspace -- -D warnings` — clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
