Rework AggregateVTable - #9816
Open
robert3005 wants to merge 8 commits into
Open
robert3005 wants to merge 8 commits into
robert3005 wants to merge 8 commits into
Conversation
Merging this PR will degrade performance by 7.86%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | arrow_checked_add_u32_neon[16384] |
12.3 µs | 20.4 µs | -39.96% |
| ⚡ | WallTime | dict_canonicalize_gt_u8_neon[1000000] |
560.2 µs | 487.5 µs | +14.93% |
| ⚡ | WallTime | words_gather_scalar_avx2[65536] |
9.4 µs | 8.3 µs | +13.38% |
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 (32d60e8) with develop (8f3dfb4)
Footnotes
-
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. ↩
robert3005
force-pushed
the
claude/aggregate-fn-vtable-refactor-ny5l4s
branch
from
September 9, 2026 17:12
de73390 to
0a82483
Compare
robert3005
force-pushed
the
claude/aggregate-fn-vtable-refactor-ny5l4s
branch
from
September 11, 2026 11:24
87b41dd to
798246e
Compare
…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
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
`From<Vec<[u32; 8]>>` existed only so one test could hand-build a saturated filter, which also let that partial disagree with its own options: it carried four blocks while the options said 256. The test now builds the filter through `partial_from_scalar` from an all-ones buffer sized to the options' block count, so the partial is consistent with the options it is checked against and nothing outside the module can bypass `BloomPartial`'s invariants. 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
force-pushed
the
claude/aggregate-fn-vtable-refactor-ny5l4s
branch
from
September 11, 2026 16:36
798246e to
6ed6e20
Compare
Drops comments that restate the line below them, narrate tests, or pad a doc with detail the signature already carries, keeping the ones that record a contract or a non-obvious decision. Two doc comments claimed that parsing and merging "inline into a single monomorphized call". That was never measured, so it is gone rather than left as an unbacked performance claim. 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
commented
Sep 14, 2026
|
|
||
| /// Return whether this stored aggregate can satisfy `requested`. | ||
| /// | ||
| /// Satisfaction is a claim about stored state, not just result semantics: consumers read this |
robert3005
commented
Sep 14, 2026
| /// all instances of the aggregate. In almost all cases, this struct will be an empty unit | ||
| /// struct, since most aggregates do not require any global state. | ||
| /// | ||
| /// Execution methods take the aggregate's options and [`AggregateDTypesRef`], which must be the |
robert3005
commented
Sep 14, 2026
robert3005
commented
Sep 14, 2026
| /// Return the partial accumulator state for an empty group. | ||
| /// The partial state of a group with no accumulated values. | ||
| /// | ||
| /// This is the identity of [`merge_partials`]: merging it with any partial state, on either |
Drops the trait-level paragraph on options and dtype lifetime, the `can_satisfy` paragraph on what satisfaction claims about stored state, and the `partial_from_scalar` note on where combining belongs. Trims the `empty_partial` identity note to one line. Signed-off-by: "Robert" <robert@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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