andygrove opened a new issue, #6002:
URL: https://github.com/apache/datafusion-comet/issues/6002
### Describe the bug
Comet's native decimal `SUM` validates the accumulator against the sum
buffer's precision on **every update** and latches the failure. Spark does not
check intermediate values at all in several common buffer layouts, so it can
recover from a temporary overflow and return a result that fits.
Where the two disagree, Comet returns `NULL` in legacy mode and raises
`ARITHMETIC_OVERFLOW` under ANSI, while Spark returns a value. The aggregate
runs entirely in Comet, so no mixed Comet/Spark plan is needed to hit this.
### Steps to reproduce
One partition, no `GROUP BY`, whole-stage codegen enabled:
```sql
CREATE TABLE t AS
SELECT CAST(v AS DECIMAL(38,38)) AS v FROM VALUES ('0.6'), ('0.6'),
('-0.6') AS s(v);
SELECT SUM(v) FROM t;
```
`DECIMAL(38,38)` matters because `Sum.resultType` is `DecimalType.bounded(p
+ 10, s)`, which saturates at `(38, 38)` and leaves no headroom. The running
sum reaches `1.2`, which does not fit in that precision, and the third value
brings it back to `0.6`, which does.
### Expected behavior
Spark returns `0.6` in both legacy and ANSI mode.
Comet returns `NULL` in legacy mode and throws `ARITHMETIC_OVERFLOW` under
ANSI.
### Additional context
**Why Spark differs.** `Sum.add` uses `DecimalAddNoOverflowCheck` for
decimal inputs. The doc comment on that expression says the overflow check is
deliberately skipped because `UnsafeRowWriter` will perform it when the
aggregation buffer is written. That only holds when the buffer actually is an
`UnsafeRow`, and it is not in at least three places:
- ungrouped `HashAggregateExec` under whole-stage codegen, which keeps the
buffer in local `Decimal` variables (`doProduceWithoutKeys`),
- window frames, where `AggregateProcessor` buffers into a
`SpecificInternalRow`,
- `ObjectHashAggregateExec`, which buffers the same way.
In all three Spark keeps the wide intermediate and only `CheckOverflowInSum`
at evaluate time decides. Grouped `HashAggregateExec` does write an
`UnsafeRow`, so Spark nulls there too and Comet agrees.
**Comet side.** `SumDecimalAccumulator::update_single`
(`native/spark-expr/src/agg_funcs/sum_decimal.rs`, around line 202) and
`SumDecimalGroupsAccumulator::update_single` (around line 421) both call
`Decimal128Type::is_valid_decimal_precision` per row, then either set the sum
to `None` permanently or raise immediately under ANSI. Once latched, a later
cancelling value cannot recover the sum.
**Scope.** This needs a decimal whose sum precision has little headroom, in
practice an input precision of 28 or more, together with an intermediate that
exceeds the buffer precision and later inputs that bring it back into range.
`CometWindowExec` has no guard for the ever-expanding decimal `SUM` case either.
**Related work.**
- PR #5420 fixes exactly this class for decimal `AVG`, by keeping global and
expanding-window decimal AVG in Spark once the sum precision reaches
`DecimalType.MAX_PRECISION`. Decimal `SUM` has no equivalent guard in either
the aggregate or the window path.
- #5975 records the `0.6, 0.6, -0.6` behaviour, but as justification for an
exclusion inside an enhancement about recovering native partial aggregate
coverage. It does not track the wrong answer itself.
- The measured control for the `DECIMAL(38,38)` case, run against a matching
native library, is in
https://github.com/apache/datafusion-comet/pull/5421#issuecomment-5504530845.
- #4729 (closed) covered a different decimal SUM window overflow, where the
result wrapped instead of returning NULL.
**Suggested fix.** Either match Spark's deferred check by keeping an
unbounded intermediate and validating once at `evaluate`, or add a precision
guard mirroring the one in #5420, covering the ungrouped aggregate, the
ever-expanding window frame, and `ObjectHashAggregate`.
--
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]
