Support out of order grouped aggregate accumulation by gatesn · Pull Request #8379 · vortex-data/vortex

gatesn · 2026-06-11T21:41:16Z

Our original API for grouped aggregate functions only supported groups that we pre-sorted into complete lists. That meant large groups had to be globally sorted before we could start accumulating their internal state, unlike the grouped-aggregate APIs used by query engines.

This PR updates the grouped accumulator API to accept caller-assigned dense GroupIds directly.

Summary

Accept GroupIds { ids: ArrayRef, num_groups }, including repeated, out-of-order, and absent IDs.
Dispatch grouped kernels on both the value encoding and group-ID encoding.
Cache materialized and validated group IDs across clones and aggregate functions.
Keep a list-group compatibility adapter for existing list_sum callers.
Let each aggregate own its dense grouped-state representation, with the existing generic partial-state implementation as the fallback.
Store grouped count state as one u64 per group.
Store grouped sum state in monomorphic u64, i64, f64, or decimal vectors, with overflow and empty state kept separately.
Preserve SQL null-on-empty sum semantics, including all-null groups and groups absent from a batch.
Select exact decimal state width (i8 through i256) once when the accumulator is created.
Merge count, primitive-sum, float-sum, and decimal-sum partial arrays without execute_scalar per row.
Adapt between direct typed scatter for unclustered IDs and per-run reduction for clustered IDs.
Use one stable counting-sort gather per morsel in the universal fallback instead of one take per group.

Spiral impact

Spiral already interns group keys into dense u32 state slots, so it can pass those IDs directly without rearranging values into ordered list groups. This covers the common count/sum machinery while allowing Spiral to retain its stronger integer/decimal mean states and other specialized paths.

Local performance

Medians below use 65,536 non-null i32 rows and include canonical result materialization. Shuffled inputs contain the same multiset of IDs as their clustered counterpart.

Groups	Count clustered	Count shuffled	Sum clustered	Sum shuffled
128	16.6 μs	16.5 μs	22.0 μs	36.3 μs
4,096	18.5 μs	16.1 μs	30.2 μs	26.9 μs
65,536	18.3 μs	37.2 μs	60.4 μs	90.2 μs

The original 65,536-row / 4,096-group shuffled sum benchmark was approximately 215 μs with per-group SumPartial state. Typed storage plus adaptive scatter reduces it to approximately 26.9 μs while also materializing the result and tracking SQL empty-state semantics, an approximately 8× improvement.

On the existing roughly 1K-row cases, representative medians are 1.9 μs for clustered-null count, 3.7 μs for nullable all-valid i32 sum, 3.9 μs for all-valid f64 sum, and 6.2 μs for clustered-null i32 sum.

Validation

cargo +nightly fmt --all
cargo test -p vortex-array --lib (3,376 passed, 1 ignored)
cargo bench -p vortex-array --bench aggregate_grouped
cargo clippy -p vortex-array --all-targets --all-features
cargo clippy --all-targets --all-features
RUSTDOCFLAGS="-D warnings" cargo doc --profile ci --no-deps -p vortex-array

Signed-off-by: "Nicholas Gates" <nick@nickgates.com>

codspeed-hq · 2026-06-11T21:54:30Z

Merging this PR will degrade performance by 57.86%

⚡ 3 improved benchmarks
❌ 12 regressed benchmarks
✅ 1925 untouched benchmarks
🆕 12 new benchmarks
⏩ 89 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`fsl_sum_medium`	1.2 ms	12.5 ms	-90.68%
❌	Simulation	`fsl_sum_large`	96.5 ms	963 ms	-89.98%
❌	Simulation	`listview_sum_medium`	1.6 ms	13.1 ms	-87.83%
❌	Simulation	`list_sum_medium`	2 ms	13.5 ms	-85.41%
❌	Simulation	`listview_sum_large`	150.5 ms	1,022.1 ms	-85.27%
❌	Simulation	`list_sum_large`	185.3 ms	1,056.4 ms	-82.46%
❌	Simulation	`list_sum_nullable_elements_large`	464.4 ms	1,340.6 ms	-65.36%
❌	Simulation	`list_sum_nullable_elements_medium`	4.7 ms	12.4 ms	-61.91%
❌	Simulation	`fsl_sum_small`	211.4 µs	320.4 µs	-34.01%
❌	Simulation	`listview_sum_small`	222.6 µs	334.4 µs	-33.43%
❌	Simulation	`list_sum_small`	314.1 µs	420.9 µs	-25.37%
❌	Simulation	`sum_i32_nullable_all_valid`	183.7 µs	204.9 µs	-10.35%
⚡	Simulation	`count_varbinview`	176.3 µs	40.5 µs	×4.3
⚡	Simulation	`varbinview_large`	1,283.4 µs	557.2 µs	×2.3
⚡	Simulation	`count_i32_clustered_nulls`	103.3 µs	73.4 µs	+40.7%
🆕	Simulation	`count_i32_cardinality[(128, Clustered)]`	N/A	541.8 µs	N/A
🆕	Simulation	`count_i32_cardinality[(128, Shuffled)]`	N/A	606.1 µs	N/A
🆕	Simulation	`count_i32_cardinality[(4096, Clustered)]`	N/A	673.9 µs	N/A
🆕	Simulation	`count_i32_cardinality[(4096, Shuffled)]`	N/A	886.2 µs	N/A
🆕	Simulation	`count_i32_cardinality[(65536, Clustered)]`	N/A	2.3 ms	N/A
...	...	...	...	...	...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing ngates/grouped-aggregate (b27ec6a) with develop (5794e7d)}

89 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. ↩

Signed-off-by: "Nicholas Gates" <nick@nickgates.com>

I, Nicholas Gates <nick@nickgates.com>, hereby add my Signed-off-by to this commit: 6bdcd32 I, Nicholas Gates <nick@nickgates.com>, hereby add my Signed-off-by to this commit: 9bd157f I, Nicholas Gates <nick@nickgates.com>, hereby add my Signed-off-by to this commit: 50701b2 Signed-off-by: Nicholas Gates <nick@nickgates.com>

joseph-isaacs · 2026-06-12T11:02:46Z

We wait that vortex was a linear scan engine. This breaks it correct?

onursatici

I prefer this API, not only that it supports out of order groups but also it can keep the memory footprint lower by not having us to materialise each group before calling aggregate.

I think you are conflicting with #8314
Sum and count kernels you added make sense to me but we went with having a encoding agnostic kernel support on the registry on the merged PR instead of having the kernel in the aggregate function's vtable

gatesn · 2026-06-12T18:19:31Z

We wait that vortex was a linear scan engine. This breaks it correct?

No, the idea is that I can take an array, compute the group indices (i.e. using a scalar function), then update the aggregate state per group. This still doesn't require a shuffle / sort. But it does mean aggregate state may grow large. So in the future this is where we would want the ability to spill partials / reconstruct later.

Signed-off-by: Nicholas Gates <nick@nickgates.com>

AdamGS · 2026-06-12T20:38:26Z

+    num_groups: usize,
+}
+
+fn dense_grouped_input(values: ArrayRef, group_sizes: &[usize]) -> DenseGroupedInput {


nit - this is basically the constructor for DenseGroupedInput

Signed-off-by: Nicholas Gates <nick@nickgates.com>

AdamGS · 2026-06-12T20:42:01Z

-fn grouped_count(
-    elements: &ArrayRef,
-    group_ranges: &GroupRanges,
+pub(super) fn try_accumulate_grouped(


nit - this seems short enough to inline into the vtable

onursatici

I think we should choose either to have encoding agnostic grouped kernels that we register in the registry, like develop does with AggregateFnSession::register_grouped_kernel, or have the fast path as methods on the aggregate function vtable, like this PR does with AggregateFnVTable::try_accumulate_grouped

If we merge this as is, we will have two ways that do very similar things

gatesn · 2026-06-14T23:33:32Z

Agreed, in general I want to move everything towards session kernels, including both aggregate functions and scalar functions.

joseph-isaacs · 2026-06-15T10:32:33Z

+    for (&group_id, valid) in group_ids.iter().zip(validity.iter()) {
+        if valid {
+            states[group_id as usize] += 1;
+        }


This also looks kinda slow

joseph-isaacs · 2026-06-15T10:33:36Z

+fn for_each_valid_idx(validity: &Mask, len: usize, mut f: impl FnMut(usize)) {
+    match validity.indices() {
+        AllOr::All => {
+            for idx in 0..len {
+                f(idx);
+            }
+        }
+        AllOr::None => {}
+        AllOr::Some(indices) => {
+            for &idx in indices {
+                f(idx);
+            }


Is this not a method on Mask?

joseph-isaacs · 2026-06-15T10:55:03Z

+        &self,
+        _states: &mut [Self::Partial],
+        _batch: &ArrayRef,
+        _group_ids: &[u32],


I think we should use an ArrayRef here so we can dispatch on different encoding ids? RLE, Constant

joseph-isaacs · 2026-06-15T10:58:31Z

-    fn push_result(&mut self, state: ArrayRef) -> VortexResult<()> {
+    fn merge_group(
+        &mut self,
+        into: u32,


Suggested change

into: u32,

into_num_groups: u32,

joseph-isaacs · 2026-06-15T10:59:10Z

+        group_ids: &[u32],
+        num_groups: usize,


Should this be a Group struct?

joseph-isaacs · 2026-06-15T10:59:38Z

+        group_ids: &[u32],
+        ctx: &mut ExecutionCtx,


Why don't we know the num_groups here?

github-actions · 2026-07-15T04:21:27Z

This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days

github-actions · 2026-07-22T04:40:47Z

This PR was closed because it has been inactive for 7 days since being marked as stale.

Signed-off-by: Nicholas Gates <nick@nickgates.com>

…regate Signed-off-by: Nicholas Gates <nick@nickgates.com> # Conflicts: # vortex-array/benches/aggregate_grouped.rs # vortex-array/src/aggregate_fn/accumulator_grouped.rs # vortex-array/src/aggregate_fn/fns/sum/grouped.rs # vortex-array/src/aggregate_fn/fns/sum/mod.rs # vortex-array/src/scalar_fn/fns/list_sum.rs

Support dense grouped aggregate accumulation

6bdcd32

Signed-off-by: "Nicholas Gates" <nick@nickgates.com>

gatesn requested a review from onursatici June 11, 2026 21:41

gatesn added the changelog/break A breaking API change label Jun 11, 2026

gatesn changed the title ~~Support dense grouped aggregate accumulation~~ Support out of order grouped aggregate accumulation Jun 11, 2026

Clarify dense grouped aggregate ids

9bd157f

Signed-off-by: "Nicholas Gates" <nick@nickgates.com>

gatesn marked this pull request as ready for review June 11, 2026 21:43

gatesn requested a review from a team June 11, 2026 21:43

gatesn enabled auto-merge (squash) June 11, 2026 21:43

gatesn added 2 commits June 11, 2026 18:04

Fix grouped aggregate CI checks

50701b2

Signed-off-by: "Nicholas Gates" <nick@nickgates.com>

gatesn requested a review from joseph-isaacs June 12, 2026 02:49

onursatici reviewed Jun 12, 2026

View reviewed changes

gatesn added 2 commits June 12, 2026 14:23

Merge origin/develop into grouped aggregate

e999a5f

Signed-off-by: Nicholas Gates <nick@nickgates.com>

Restore dense grouped aggregate test coverage

c61b7bb

Signed-off-by: Nicholas Gates <nick@nickgates.com>

AdamGS reviewed Jun 12, 2026

View reviewed changes

Optimize dense grouped aggregates

2ef64b2

Signed-off-by: Nicholas Gates <nick@nickgates.com>

AdamGS reviewed Jun 12, 2026

View reviewed changes

onursatici reviewed Jun 14, 2026

View reviewed changes

joseph-isaacs reviewed Jun 15, 2026

View reviewed changes

group ids as array ref, multi encoding kernel lookup (#8550)

4ba3988

github-actions Bot added the stale This PR is stale and will be auto-closed soon label Jul 15, 2026

github-actions Bot closed this Jul 22, 2026

auto-merge was automatically disabled July 22, 2026 04:40
Pull request was closed

gatesn reopened this Aug 7, 2026

gatesn added 4 commits August 7, 2026 18:55

Merge develop and complete grouped aggregates

9022490

Signed-off-by: Nicholas Gates <nick@nickgates.com>

Optimize grouped aggregate hot paths

3560666

Signed-off-by: Nicholas Gates <nick@nickgates.com>

Optimize grouped aggregate state storage

f52c15d

Signed-off-by: Nicholas Gates <nick@nickgates.com>

gatesn marked this pull request as draft August 8, 2026 02:05

github-actions Bot removed the stale This PR is stale and will be auto-closed soon label Aug 9, 2026

Uh oh!

Conversation

gatesn commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Spiral impact

Local performance

Validation

Uh oh!

codspeed-hq Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 57.86%

Performance Changes

Footnotes

Uh oh!

joseph-isaacs commented Jun 12, 2026

Uh oh!

onursatici left a comment

Choose a reason for hiding this comment

Uh oh!

gatesn commented Jun 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onursatici left a comment

Choose a reason for hiding this comment

Uh oh!

gatesn commented Jun 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 15, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gatesn commented Jun 11, 2026 •

edited

Loading

codspeed-hq Bot commented Jun 11, 2026 •

edited

Loading