dwsmith1983 commented on code in PR #6041:
URL: https://github.com/apache/datafusion-comet/pull/6041#discussion_r4053647587
##########
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?
Done in e5bd4cc. Spark turns codegen off for a hash aggregate when any
function is an `ImperativeAggregate`, or when `spark.sql.codegen.wholeStage` is
off, and then buffers the ungrouped sum in an UnsafeRow that latches.
`CometHashAggregateExec` now declines an ungrouped decimal SUM at precision 38
in both cases with a mode-neutral reason, and the new test covers your `SUM(v),
approx_count_distinct(k)` shape and the config switch, in legacy mode (NULL
with the fallback reason recorded) and under ANSI (both engines fail). A
`DECIMAL(10,2)` control stays native. The cases Comet cannot see at planning, a
stage over `spark.sql.codegen.maxFields` or a codegen compile fallback, remain
in the compatibility note.
--
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]
