0lai0 opened a new pull request, #6764:
URL: https://github.com/apache/datafusion-comet/pull/6764
## Which issue does this PR close?
Closes #6661
## Rationale for this change
With `spark.sql.ansi.enabled=true`, Comet raised `ARITHMETIC_OVERFLOW` on an
integral `SUM` overflow with the message `integer overflow` and an empty
`alternative`. Spark reports `long overflow` with the suggestion `Use 'try_add'
to tolerate overflow and return NULL instead.`
Spark's `SUM` over integral input always returns `LONG` and adds through
`Add(left, right, evalContext)`. The overflow therefore goes through
`MathUtils.addExact(Long, Long, context)`, which passes `try_add` as the hint,
whatever the input type. Comet already throws in the same cases. Only the
message parameters differ, and that matters to code that inspects
`getMessageParameters()` or the message text.
## What changes are included in this PR?
- Add `long_add_overflow_error()` to `native/spark-expr/src/lib.rs`. It
returns `SparkError::ArithmeticOverflow { from_type: "long", function_name:
"try_add" }`, using the `function_name` field that #6249 added.
- Use it at the three overflow sites in `sum_int.rs`: the ungrouped update
(the ungrouped merge reuses it), the grouped update, and the grouped merge. The
other callers of `arithmetic_overflow_error` are unchanged.
- `planner.rs` also uses `SumInteger` for window `SUM` over ever-expanding
frames, so those windows now report the Spark error too. Sliding frames go
through DataFusion's built-in `sum` and are not changed here. That path still
ignores ANSI and returns a wrapped value instead of throwing, which #6043
tracks.
## How are these changes tested?
- New Rust unit tests in `sum_int.rs` pin `from_type == "long"` and
`function_name == "try_add"` on every overflow path.
- The two ANSI `SUM` tests in `CometAggregateSuite` used to pass with or
without the fix. They now use `checkSparkError` and check that `alternative`
carries `try_add`. They cover overflow and underflow, grouped and ungrouped
queries, and overflow in the partial aggregate and in the final merge.
- Both Scala tests pass under `-Pspark-3.4` and `-Pspark-4.1`, and fail with
the native change reverted.
--
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]
