Skip to content

Deduplicate Legacy/Ansi/Try integer SUM accumulators in sum_int.rs #6976

Open

Description

@andygrove

What is the problem the feature request solves?

native/spark-expr/src/agg_funcs/sum_int.rs has six accumulator implementations, Legacy/Ansi/Try for both the row (SumIntegerAccumulator*) and grouped (SumIntGroupsAccumulator*) forms, and they share almost all of their code:

  • Each update_batch defines a local generic sum loop and then repeats the same four-arm Int64/Int32/Int16/Int8 dispatch with the same unsupported-type error (row accumulators around L119-154, L190-227, L273-316; groups accumulators at L404-473, L550-622, L711-793).
  • evaluate, state, convert_to_state and size are identical between the Legacy and Ansi groups accumulators, and merge_batch differs only in the add.
  • The real difference between the modes is one operation: add_wrapping (Legacy), add_checked mapped to an overflow error (Ansi), and add_checked mapped to None (Try). Try also tracks has_all_nulls, which is a legitimate difference.

Adding an integer type, or fixing filter or null handling, means editing six places, and the Try variant is the one most likely to drift.

Describe the potential solution

  • Hoist the type dispatch into one helper (e.g. dispatch_int_array(values, |arr| ...)) so the Int8 to Int64 match exists once.
  • Write one generic sum loop that takes the add operation (impl Fn(i64, i64) -> Option<i64>, or an OverflowMode trait), with Legacy, Ansi and Try passing their own add.
  • Make the groups accumulator generic over the mode so Legacy and Ansi share evaluate/state/size/merge_batch, and keep Try's has_all_nulls handling as a separate impl or mode.

Additional context

Line references are pinned to commit 7d8d9b1.

Activity

  1. added
    enhancementNew feature or request
    rustPull requests that update Rust code
    area:aggregationHash aggregates, aggregate expressions
    help wantedExtra attention is needed
    on Oct 14, 2026
  2. ulagsd commented on Nov 11, 2026

    @ulagsd

    take

    Plan:

    • Hoist the Int8–Int64 dispatch into one helper.
    • One generic sum loop parameterized by the add (wrapping / checked→error / checked→None).
    • Make the groups accumulator generic over the mode, keeping Try's has_all_nulls separate.

    Following the benchmarks in #5098, I'll keep per-width loops (no widening to an Int64Array).

    Baseline on main (e59d937, Apple M5), cargo bench --bench aggregate -- sum_integer:

    Legacy ANSI Try
    end-to-end Int64 411 µs 446 µs 485 µs
    row accumulator 5.4 µs 22.0 µs 20.9 µs
    groups accumulator 43.1 µs 48.5 µs 80.5 µs

    The existing benches only cover Int64, while the #5098 regressions were on narrow types, so I'd also add Int8/Int16/Int32 cases to benches/aggregate.rs as a first commit and report before/after for every width.

  3. ulagsd commented on Nov 11, 2026

    @ulagsd

    take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area:aggregationHash aggregates, aggregate expressionsenhancementNew feature or requesthelp wantedExtra attention is neededrustPull requests that update Rust codetech debt

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions