apache / apache/datafusion-comet

sum_int: use arrow sum kernels and collapse integer type dispatch

Open
#5,098 3 comments 0 reactions 1 assignee Claimed by @Smallfu666 View on GitHub
area:aggregation enhancement
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 6h
Merged PRs (30d)
190

Description

### What is the problem the feature request solves?

All six accumulators in `native/spark-expr/src/agg_funcs/sum_int.rs` repeat an identical Int8/Int16/Int32/Int64 downcast dispatch (six copies, roughly 150 lines) and hand-rolled per-element loops:

- Legacy: row-by-row `to_i64()` plus `add_wrapping` into the running sum.
- ANSI: same with `add_checked`, mapping overflow to the Spark arithmetic overflow error.
- Try: same with overflow mapping to a sticky null.

Arrow provides the batch cores: `cast` to Int64 once (widening, lossless), then `arrow::compute::sum` (wrapping, null-skipping, returns `None` for all-null, matching the existing early return) or `sum_checked`, then a single `add_wrapping`/`add_checked` against the running sum. This collapses the dispatch to one path and gets SIMD.

### Describe the potential solution

- Legacy accumulator: safe to switch outright. Wrapping addition is associative and commutative, so batch-sum-then-add is bit-identical to element-by-element accumulation.
- ANSI and Try accumulators: switching changes accumulation order (batch first, running sum last), which affects one edge case: an input whose running prefix transiently overflows but whose total fits. The current row-ordered code (and Spark) errors (ANSI) or yields null (Try); the batch approach would succeed. Decide whether to accept that divergence or keep those two loops row-ordered.
- The three GroupsAccumulators keep their per-group loops (arrow has no grouped kernels) but can all operate on one pre-cast `Int64Array`, removing the per-width dispatch.

### Additional context

Found during an audit of native code that replicates existing arrow-rs kernels.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.