Skip to content

Conversation

@andygrove
Copy link
Member

@andygrove andygrove commented Jan 8, 2026

Which issue does this PR close?

Closes #.

Rationale for this change

The current implementation of accumulators for sum_int handle all three eval modes, making the code complex to understand and also adds overhead due to matching on eval mode within loops.

What changes are included in this PR?

Refactored sum_int.rs from a single accumulator with eval mode branches inside loops into 6 separate accumulator implementations:

Single-row accumulators:

  • SumIntegerAccumulatorLegacy - uses wrapping arithmetic (no overflow checking)
  • SumIntegerAccumulatorAnsi - uses checked arithmetic, returns error on overflow
  • SumIntegerAccumulatorTry - uses checked arithmetic, returns None on overflow, tracks has_all_nulls

Groups accumulators:

  • SumIntGroupsAccumulatorLegacy
  • SumIntGroupsAccumulatorAnsi
  • SumIntGroupsAccumulatorTry

Key changes:

  • SumInteger::accumulator() and create_groups_accumulator() now use a match on eval_mode to instantiate the appropriate accumulator type
  • Each accumulator has a dedicated inner update_sum function without any eval mode branching
  • The Legacy/Ansi accumulators don't carry the has_all_nulls field since they don't need it
  • Added helper methods like overflowed() and group_overflowed() for the Try variants to improve readability

How are these changes tested?

Existing tests.

Benchmark results show some improvement for group accumulators but not for row accumulators.

sum_integer_accumulator/groups_legacy
                        time:   [53.969 µs 54.108 µs 54.283 µs]
                        change: [−14.477% −12.994% −11.194%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  2 (2.00%) high mild
  7 (7.00%) high severe
sum_integer_accumulator/groups_ansi
                        time:   [59.963 µs 60.076 µs 60.189 µs]
                        change: [−30.722% −30.522% −30.325%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
Benchmarking sum_integer_accumulator/groups_try: Warming up for 500.00 ms
Warning: Unable to complete 100 samples in 500.0ms. You may wish to increase target time to 520.5ms, enable flat sampling, or reduce sample count to 60.
sum_integer_accumulator/groups_try
                        time:   [100.99 µs 101.23 µs 101.45 µs]
                        change: [−7.5070% −6.2309% −5.2479%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high severe

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.50%. Comparing base (f09f8af) to head (26b852d).
⚠️ Report is 833 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3054      +/-   ##
============================================
+ Coverage     56.12%   59.50%   +3.37%     
- Complexity      976     1381     +405     
============================================
  Files           119      167      +48     
  Lines         11743    15522    +3779     
  Branches       2251     2575     +324     
============================================
+ Hits           6591     9236    +2645     
- Misses         4012     4989     +977     
- Partials       1140     1297     +157     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andygrove andygrove marked this pull request as ready for review January 8, 2026 20:11
@andygrove andygrove changed the title perf: refactor sum int with specialized implementations for each eval_mode [WIP] perf: refactor sum int with specialized implementations for each eval_mode Jan 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants