Skip to content

Cut over sum aggregate function#6910

Open
gatesn wants to merge 6 commits intodevelopfrom
ngates/sum-agg
Open

Cut over sum aggregate function#6910
gatesn wants to merge 6 commits intodevelopfrom
ngates/sum-agg

Conversation

@gatesn
Copy link
Contributor

@gatesn gatesn commented Mar 12, 2026

Move sum compute function over to the Sum impl of AggregatFnVTable

Note that we remove the ability to sum extension types since we were incorrectly just summing the storage DType. This doesn't make sense, e.g. summing timestamps is invalid, but summing time deltas is allowed.

Signed-off-by: Nicholas Gates <nick@nickgates.com>
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 12, 2026

Merging this PR will degrade performance by 10.2%

⚡ 6 improved benchmarks
❌ 1 regressed benchmark
✅ 1002 untouched benchmarks
⏩ 1515 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation take_map[(0.1, 1.0)] 3.8 ms 3.5 ms +10.6%
Simulation null_count_run_end[(10000, 256, 0.1)] 41.6 µs 37.4 µs +11.07%
Simulation null_count_run_end[(10000, 256, 0.5)] 41.7 µs 37.2 µs +12.23%
Simulation null_count_run_end[(10000, 4, 0.01)] 140.5 µs 113.7 µs +23.56%
Simulation null_count_run_end[(10000, 1024, 0.1)] 40 µs 35.7 µs +12.18%
Simulation null_count_run_end[(10000, 1024, 0.5)] 40.1 µs 35.5 µs +12.79%
Simulation map_each[BufferMut<i32>, 128] 770.6 ns 858.1 ns -10.2%

Comparing ngates/sum-agg (8ae7b12) with develop (da4c196)

Open in CodSpeed

Footnotes

  1. 1515 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@robert3005
Copy link
Contributor

We should make sure we port any exhisting behaviour we have in the current implementations

@gatesn
Copy link
Contributor Author

gatesn commented Mar 12, 2026

Like what?

@joseph-isaacs
Copy link
Contributor

NaN handlings at least. Its work fuzzing this on the side is possible

@robert3005
Copy link
Contributor

I left a comment on the pr that added sum with the code location since that code isn’t touched in this pr. If we want to change decimal sum to always work in i256 we should make this in a separate pr

@robert3005
Copy link
Contributor

FWIW NaNs are a long standing issue #5152. I think fixing NaN handling is what's actually required for this stat to be universally useful but it didn't change in this pr

gatesn added 2 commits March 13, 2026 11:33
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
@gatesn gatesn added changelog/feature A new feature feature A feature request labels Mar 13, 2026
@gatesn gatesn marked this pull request as ready for review March 13, 2026 16:39
gatesn added 3 commits March 13, 2026 13:18
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
@gatesn gatesn enabled auto-merge (squash) March 13, 2026 17:26
@gatesn
Copy link
Contributor Author

gatesn commented Mar 13, 2026

Added bonus, benchmarks are 10-15% faster!

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

Labels

changelog/feature A new feature feature A feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants