Skip to content

Aggregate Fns#6721

Merged
gatesn merged 10 commits intodevelopfrom
ngates/aggregate-fns
Mar 6, 2026
Merged

Aggregate Fns#6721
gatesn merged 10 commits intodevelopfrom
ngates/aggregate-fns

Conversation

@gatesn
Copy link
Copy Markdown
Contributor

@gatesn gatesn commented Feb 28, 2026 •
edited
Loading

First PR implementing the Aggregate Functions proposal in vortex-data/rfcs#21

@gatesn
Aggregate Fns
a17f2a8 
Signed-off-by: Nicholas Gates <[email protected]>
@gatesn Graphite App
Copy link
Copy Markdown
Contributor Author

gatesn commented Feb 28, 2026 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@gatesn gatesn added the changelog/feature A new feature label Feb 28, 2026
@gatesn
Aggregate Fns
6d9bcac 
Signed-off-by: Nicholas Gates <[email protected]>
This was referenced Feb 28, 2026
Closed
Closed
Closed
joseph-isaacs
joseph-isaacs reviewed Mar 2, 2026
View reviewed changes
gatesn added 6 commits March 5, 2026 10:55
@gatesn
Merge branch 'develop' into ngates/aggregate-fns
e23cc82
@gatesn
mergE
13902b2 
Signed-off-by: Nicholas Gates <[email protected]>
@gatesn
mergE
1691a91 
Signed-off-by: Nicholas Gates <[email protected]>
@gatesn
mergE
fe20d27 
Signed-off-by: Nicholas Gates <[email protected]>
@gatesn
mergE
785821b 
Signed-off-by: Nicholas Gates <[email protected]>
@gatesn
mergE
ba91c55 
Signed-off-by: Nicholas Gates <[email protected]>
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 6, 2026 •
edited
Loading

Merging this PR will improve performance by 22.76%

⚡ 2 improved benchmarks
✅ 392 untouched benchmarks
⏩ 2052 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation take_map[(0.1, 0.5)] 2.6 ms 2.1 ms +22.76%
⚡ Simulation take_map[(0.1, 1.0)] 4.2 ms 3.6 ms +16.84%

Comparing ngates/aggregate-fns (828f62c) with develop (5d6a3c8)2

Open in CodSpeed

Footnotes

  1. 2052 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. ↩

  2. No successful run was found on develop (e719a17) during the generation of this report, so 5d6a3c8 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

gatesn added 2 commits March 6, 2026 10:21
@gatesn
mergE
f901b01 
Signed-off-by: Nicholas Gates <[email protected]>
@gatesn
mergE
828f62c 
Signed-off-by: Nicholas Gates <[email protected]>
@gatesn gatesn marked this pull request as ready for review March 6, 2026 15:23
@gatesn gatesn enabled auto-merge (squash) March 6, 2026 15:23
@gatesn gatesn merged commit 761c404 into develop Mar 6, 2026
49 checks passed
@gatesn gatesn deleted the ngates/aggregate-fns branch March 6, 2026 16:25
robert3005
robert3005 reviewed Mar 12, 2026
View reviewed changes
vortex-array/src/aggregate_fn/fns/sum.rs
AllOr::None => Ok(false),
AllOr::All => match_each_decimal_value_type!(d.values_type(), |T| {
for &v in d.buffer::<T>().iter() {
match acc.checked_add(&DecimalValue::from(v)) {
Copy link
Copy Markdown
Contributor

@robert3005 robert3005 Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be forcing upcasting here. Just because you have Decimal it doesn't mean you should do all compute in i256 space

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

Reviewers

@robert3005 robert3005 robert3005 left review comments

@joseph-isaacs joseph-isaacs joseph-isaacs approved these changes

Assignees

No one assigned

Labels

changelog/feature A new feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gatesn @robert3005 @joseph-isaacs