Repository navigation
sum_int: use arrow sum kernels and collapse integer type dispatch #5098
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()plusadd_wrappinginto 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.
Activity
Smallfu666 commentedon Jul 31, 2026
take
fix: use element-wise add_wrapping in legacy sum_int to prevent debug…
refactor: use arrow sum kernels and collapse integer type dispatch in…
Smallfu666 commentedon Aug 3, 2026
I benchmarked the straightforward implementation proposed here: cast each input batch to
Int64, then use Arrow's sum kernel for scalar aggregation and a per-element loop over the widenedInt64Arrayto accumulate per-group sums for grouped aggregation.Across Int8/Int16/Int32/Int64, scalar/grouped, and Legacy/ANSI/Try configurations (Criterion, 100 samples, repeated in two independent runs), the widened-array approach consistently regressed narrow-integer performance: approximately 75–322% for scalar aggregation and 40–97% for grouped aggregation. Int64 was roughly neutral.
The main cost appears to be materializing and writing a widened
Int64Arrayfor every batch, which the current per-width loops avoid. Given that this trades a substantial performance regression for dispatch cleanup, I am dropping this implementation and unassigning myself.A viable follow-up would likely need either a zero-allocation widening reduction that accumulates narrow inputs directly into
i64, or a dispatch-deduplication refactor that retains the current per-width loops. I can provide the benchmark matrix if useful.coderfender commentedon Aug 3, 2026
Let me also run the criterion benchmark and post the numbers here
andygrove commentedon Sep 28, 2026
The arrow sum kernel route was benchmarked above at 75-322% slower for narrow integer inputs, because it has to materialize a widened Int64Array per batch, and it is neutral for Int64, so it is not worth doing. I think we can close this as not planned; a zero-allocation widening loop or a dispatch cleanup that keeps the per-width loops can be a separate issue.
Smallfu666 commentedon Sep 28, 2026
Agreed, thanks for following up.