apache / apache/datafusion-comet
sum_int: use arrow sum kernels and collapse integer type dispatch
- 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
Assessment
This issue has not been assessed yet.