Rework AggregateVTable by robert3005 · Pull Request #9816 · vortex-data/vortex

robert3005 · 2026-09-09T16:59:22Z

Most methods now also take options thus partial state doesn't need to persist options necessary on each invocation. We separate out parse/merge operation on partial state such that we can perform operations in Partial type and also can easily validate serialisation of Partial into scalar

…duce_partials The vtable's `empty_partial`, `combine_partials` (typed partial plus untyped scalar) and `reset` are replaced by two primitives so each aggregate either parses scalars or operates on typed state: - `partial_from_scalar(options, input_dtype, scalar) -> Partial` parses a partial scalar (kernel results, cached statistics, other accumulators' `to_scalar`) into the typed state, and is its inverse. - `reduce_partials(options, input_dtype, impl IntoIterator<Item = Partial>)` reduces owned partials in iteration order; the empty sequence is the identity (the state of a group with no values). Accumulator changes: - `Accumulator.partial` is an `Option`, with `None` as the cheap empty state, so folds and empty accumulators never construct an identity partial. - `DynAccumulator::combine_partials` is removed. `merge_from` downcasts the other accumulator (`DynAccumulator::downcast_mut`), checks that the aggregate, options and input dtype match, and folds its typed partial directly, so merging never round-trips through scalars. - `Combined` holds typed child accumulators and merges children with `merge_from`; its `reduce_partials` seeds from the first pair instead of building fresh children per fold. Partial states: - Min, Max, MinMax, BoundedMin, BoundedMax, IsSorted, IsConstant, Sum and SumV2 build their identity from one `empty` constructor, and reduce bodies move values instead of cloning them. - IsSorted and IsConstant no longer serialize a false verdict without a first value as the null (empty) struct, which previously lost the verdict when merged into a materialized empty state or persisted as a zone stat. IsSorted's reduce honors an unsorted partial before the emptiness check. - BloomPartial gains a typed `union`, so reducing bloom partials ORs blocks without serializing. `can_satisfy` documents that satisfaction is a claim about stored state: a partial for the requested aggregate must be creatable from this aggregate's partial. DuckDB's `finalize_scan` merges thread-local accumulators with `merge_from`. Tests construct partials directly where parsing is not the subject, and cover merging into a materialized empty state, boundary-less false verdicts, type-erased Combined merges, and SumV2 identity/overflow merges. Signed-off-by: "Claude" <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd

Every AggregateFnVTable execution method now receives the bound options and the resolved input/partial/result dtypes, so partial states only hold accumulated values instead of copies of the options and dtypes. - Add `AggregateDTypes<'a>` (borrowed) and `OwnedAggregateDTypes`, resolved once by `Accumulator`/`GroupedAccumulator` and lent to every vtable call. - Split reduction into typed primitives: `empty_partial` (the identity) and binary `merge_partials`; `reduce_partials` becomes a provided fold. - Add `DynAccumulator::combine_partial_scalar`, which parses a partial scalar and merges it through the typed vtable in one monomorphized call; `Accumulator` uses the same path for kernel results and cached statistics. - Strip options/dtype fields from every partial (Sum, SumV2, Count, Min/Max/MinMax, BoundedMin/Max, IsSorted, IsConstant, First/Last, BloomPartial's hash_fn, ...). `Count`'s partial is now a plain `u64`. - `BinaryCombined::return_dtype`/`finalize`/`finalize_scalar` take the combined options and dtypes; `Mean` derives its target dtype from them. Signed-off-by: "Claude" <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd

codspeed-hq · 2026-09-09T17:08:36Z

Merging this PR will regress 3 benchmarks

⚠️

Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 7 improved benchmarks
❌ 3 regressed benchmarks
✅ 2187 untouched benchmarks
⏩ 218 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`decode_primitives[f32, (1000, 512)]`	39.2 µs	57.8 µs	-32.16%
❌	Simulation	`random_i16[0.95]`	77.5 µs	95.4 µs	-18.76%
❌	Simulation	`decompress[u64, (4000, 1024)]`	70.8 µs	86.2 µs	-17.85%
⚡	WallTime	`arrow_checked_add_u32_neon[16384]`	20.4 µs	12.3 µs	+65.93%
⚡	Simulation	`random_i8[0.5]`	91.4 µs	68 µs	+34.48%
⚡	WallTime	`filtered_owned_i64_avx512[OneNullInEight]`	26.4 µs	22.3 µs	+18.29%
⚡	WallTime	`dict_canonicalize_gt_u8_neon[16000000]`	9.4 ms	8.1 ms	+16.16%
⚡	WallTime	`dict_canonicalize_gt_u8_neon[1000000]`	562.7 µs	487.2 µs	+15.51%
⚡	WallTime	`mul_u32_nonnull_avx512`	6.2 µs	5.6 µs	+11.78%
⚡	Simulation	`allocate_drop_bytes[0]`	575.7 ns	521.6 ns	+10.39%

Tip

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

_{Comparing claude/aggregate-fn-vtable-refactor-ny5l4s (87b41dd) with develop (c93f5a9)}

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

`VarBinBuilder::with_capacity` was deprecated when allocators were propagated through builders (#9670), after the btrblocks binary scheme (#9576) was written against the old API. CI denies warnings, so pass the execution context's allocator instead. Signed-off-by: "Claude" <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd

joseph-isaacs · 2026-09-10T10:30:18Z

+#[derive(Clone, Copy, Debug)]
+pub struct AggregateDTypes<'a> {
+    /// The dtype of the values being aggregated.
+    pub input: &'a DType,
+    /// The dtype of the partial state scalar, as reported by [`AggregateFnVTable::partial_dtype`].
+    pub partial: &'a DType,
+    /// The dtype of the final aggregate result, as reported by [`AggregateFnVTable::return_dtype`].
+    pub result: &'a DType,
+}
+
+/// Owned [`AggregateDTypes`], resolved once from an aggregate's options and input dtype.
+#[derive(Clone, Debug)]
+pub struct OwnedAggregateDTypes {
+    input: DType,
+    partial: DType,
+    result: DType,
+}


I prefer AggregateDTypesRef and AggregateDTypes vs Owned, but not strongly

joseph-isaacs · 2026-09-10T10:30:34Z

+    pub fn input(&self) -> &DType {
+        &self.input
+    }
+
+    /// The dtype of the partial state scalar.
+    pub fn partial(&self) -> &DType {
+        &self.partial
+    }
+
+    /// The dtype of the final aggregate result.
+    pub fn result(&self) -> &DType {
+        &self.result
+    }


joseph-isaacs · 2026-09-10T10:32:04Z

+    /// Parse a partial scalar and merge it into this accumulator's state.
    ///
-    /// The scalar must have the dtype reported by the vtable's `partial_dtype` for the
-    /// options and input dtype used to construct this accumulator.
-    fn combine_partials(&mut self, other: Scalar) -> VortexResult<()>;
+    /// The scalar must have the dtype reported by the vtable's `partial_dtype` for this
+    /// accumulator's options and input dtype, and represents input following the input already
+    /// accumulated. Parsing and merging both run through the typed vtable, so they inline into
+    /// a single monomorphized call per aggregate.
+    fn combine_partial_scalar(&mut self, partial: Scalar) -> VortexResult<()>;


why rename?

great question

joseph-isaacs · 2026-09-10T10:32:49Z

+    pub(crate) fn fold_partial(&mut self, other: V::Partial) -> VortexResult<()> {
+        self.partial = Some(match self.partial.take() {
+            // Merging the incoming partial with the empty state is the identity.
+            None => other,
+            Some(current) => {
+                self.vtable
+                    .merge_partials(&self.options, self.dtypes.borrow(), current, other)?
+            }
+        });
+        Ok(())
+    }
+
+    /// Parse a partial scalar of dtype `dtypes.partial` and merge it into the current state.
+    ///
+    /// Both steps go through the typed vtable of `V`, so they inline into one monomorphized call.
+    fn fold_partial_scalar(&mut self, scalar: Scalar) -> VortexResult<()> {
+        let other = self
+            .vtable
+            .partial_from_scalar(&self.options, self.dtypes.borrow(), scalar)?;
+        self.fold_partial(other)
+    }


do we need all these fns?

probably not, I merged both of our branches and I think there's quite a bit of redundancy now

Renames the resolved-dtype pair so the owned type carries the plain name and the borrowed view is suffixed: - `OwnedAggregateDTypes` -> `AggregateDTypes` - `AggregateDTypes<'a>` -> `AggregateDTypesRef<'a>` Its fields and accessors keep the names they had on the accumulators they were lifted from: `dtype`, `return_dtype`, and `partial_dtype`. Also removes `AggregateFnVTable::reduce_partials`. It was a provided fold over `empty_partial` and `merge_partials` with no non-test callers, so the tests now merge directly, which exercises the binary operation rather than the fold wrapper. Merging with the empty partial is the identity, so a two-element reduction is exactly one `merge_partials`. Signed-off-by: "Claude" <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd

… fields `DynAccumulator::combine_partials` took a single `Scalar`, so the plural was misleading; the sibling `merge_from` already distinguishes combining another accumulator from combining one partial scalar. `AggregateDTypes` now exposes `dtype`, `return_dtype` and `partial_dtype` as public fields, matching `AggregateDTypesRef`, and drops the three accessors that only returned them. `try_new` and `borrow` stay, since they resolve the dtypes through the vtable and lend them out rather than just reading a field. Signed-off-by: "Robert" <robert@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd

Tests that needed an empty partial were spelling out the identity state by hand, restating in the test what each vtable already defines. They now call `empty_partial`, so a change to an aggregate's identity state cannot leave a test asserting against a stale hand-written one. Covers First, Last, BoundedMin, BoundedMax, GeometryAabb and BloomFilter. First and Last no longer touch their partial structs at all: the non-empty cases go through `partial_from_scalar`, which for both is exactly a wrapped non-null scalar. `Accumulator::empty_partial` becomes `pub(crate)`, matching the existing visibility of `fold_partial`, so the bounded min/max tests can fold an empty partial without rebuilding the dtypes. Left alone: the is_sorted and is_constant partials, whose tests assert a non-empty verdict is distinguishable from the empty state and so must build it directly; the non-identity partials in sum, min_max and the decimal sum tests; and the BloomPartial unit tests, which exercise the partial type itself rather than the aggregate. Signed-off-by: "Robert" <robert@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd

robert3005 added the changelog/break A breaking API change label Sep 9, 2026

robert3005 force-pushed the claude/aggregate-fn-vtable-refactor-ny5l4s branch from de73390 to 0a82483 Compare September 9, 2026 17:12

joseph-isaacs reviewed Sep 10, 2026

View reviewed changes

claude added 3 commits September 10, 2026 17:45

Navigation Menu

Uh oh!

Rework AggregateVTable - #9816

Rework AggregateVTable#9816
robert3005 wants to merge 6 commits into
developfrom
claude/aggregate-fn-vtable-refactor-ny5l4s

robert3005 commented Sep 9, 2026

Uh oh!

codspeed-hq Bot commented Sep 9, 2026 •

edited

Loading

Uh oh!

joseph-isaacs Sep 10, 2026

Uh oh!

joseph-isaacs Sep 10, 2026

Uh oh!

joseph-isaacs Sep 10, 2026

Uh oh!

robert3005 Sep 10, 2026

Uh oh!

joseph-isaacs Sep 10, 2026

Uh oh!

robert3005 Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

robert3005 commented Sep 9, 2026

Uh oh!

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

Uh oh!

Merging this PR will regress 3 benchmarks

Performance Changes

Footnotes

Uh oh!

joseph-isaacs Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

joseph-isaacs Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

joseph-isaacs Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

robert3005 Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

joseph-isaacs Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

robert3005 Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq Bot commented Sep 9, 2026 •

edited

Loading