rich7420 commented on code in PR #6041:
URL: https://github.com/apache/datafusion-comet/pull/6041#discussion_r4053481817
##########
native/spark-expr/src/agg_funcs/sum_decimal.rs:
##########
@@ -200,28 +205,26 @@ impl SumDecimalAccumulator {
}
fn update_single(&mut self, values: &Decimal128Array, idx: usize) ->
DFResult<()> {
- // If already overflowed (sum is None but not empty), stay in overflow
state
- if !self.is_empty && self.sum.is_none() {
- return Ok(());
- }
-
let v = unsafe { values.value_unchecked(idx) };
- let running_sum = self.sum.unwrap_or(0);
- let (new_sum, is_overflow) = running_sum.overflowing_add(v);
-
- if is_overflow || !Decimal128Type::is_valid_decimal_precision(new_sum,
self.precision) {
- if self.eval_mode == EvalMode::Ansi {
- let error = decimal_sum_overflow_error("sum");
- return Err(self.wrap_error_with_context(error));
- }
- self.sum = None;
- self.is_empty = false;
- return Ok(());
- }
+ self.add_unbounded(i256::from_i128(v));
Review Comment:
Could we preserve Spark’s behavior when a sibling aggregate disables
codegen, and add a regression? On Spark 4.1.3 with `wholeStage=true`, AQE off,
and one ordered partition containing `DECIMAL(38,38)` values `0.6, 0.6, -0.6`,
`SELECT SUM(v), approx_count_distinct(k) FROM t` returns NULL in Spark but 0.6
natively. Under ANSI, Spark throws while Comet succeeds.
`approx_count_distinct` disables codegen for the aggregate, so Spark still uses
an `UnsafeRow` buffer here.
##########
docs/source/user-guide/latest/compatibility/index.md:
##########
@@ -145,6 +145,12 @@ so users hunting an unexpected value have a single place
to check:
- Native `RANGE` window frames with an explicit `PRECEDING` / `FOLLOWING`
offset diverge from
Spark when the boundary arithmetic overflows for `DATE` or `DECIMAL` `ORDER
BY` columns
([#5022](https://github.com/apache/datafusion-comet/issues/5022)).
+- Ungrouped decimal `SUM` keeps an unbounded intermediate and checks the
result precision only
+ when a partial is written out or the sum is evaluated, which matches Spark's
whole-stage codegen
+ path. With `spark.sql.codegen.wholeStage=false` Spark's ungrouped aggregate
buffers in an
+ `UnsafeRow` and latches as soon as a running sum leaves the precision, so an
intermediate
+ overflow that later cancels out returns `NULL` (or raises under ANSI) in
Spark but the recovered
+ value in Comet. Spark's own decimal overflow tests accept either outcome.
Review Comment:
Could we remove this claim? Spark’s `assertDecimalSumOverflow` accepts two
overflow error messages under ANSI; it still requires NULL in legacy mode.
```suggestion
value in Comet.
```
--
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]
