Repository navigation
feat(aggregate): register is_sorted and min_max for temporal and UUID extension types - #10616
Conversation
…ion storage Add a StorageLifter trait and an ExtensionDelegate<L> adapter that implements AggregateFnVTable for an extension dtype by running the lifter's storage aggregate over the extension's storage array and lifting the final result back. Route the existing hand-written storage delegations through it, with one lifter per aggregate: min_max, is_constant, is_sorted, uncompressed_size_in_bytes and the zoned bloom filter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
…A, E>
Replace the StorageLifter trait and ExtensionDelegate with a single
ExtensionAggregate<A, E> adapter, which defines aggregate A over
extension type E by running A over E's storage. It is an aggregate of
its own, with id `<A>[<E>]`, so supporting a new pair is one session
registration:
session.aggregate_fns().register(ExtensionAggregate::new(Sum, Timestamp));
Restore the original storage delegation in min_max, is_constant,
is_sorted, uncompressed_size_in_bytes and the zoned bloom filter.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD
Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…ate (#10612) Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Claude <noreply@anthropic.com>
…n types Register ExtensionAggregate<IsSorted, E> and ExtensionAggregate<MinMax, E> in the default aggregate session for Date, Time and Timestamp, whose values order like their integer storage, through a new AggregateFnSession::register_storage_ordered helper. Remove IsSorted's extension branch: IsSorted rejects extension dtypes in return_dtype, so the branch was unreachable; extension inputs now go through the registered adapters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
…aggregate Document, per aggregate, the property the storage must share with the extension type before a pair is registered, with examples that fit and that do not, and add a doctest that registers and runs ExtensionAggregate<IsSorted, Timestamp>. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
Merging this PR will improve performance by 19.44%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ |
Simulation | decode_primitives[i64, (1000, 8)] |
37.5 µs | 23.8 µs | +57.32% |
| ⚡ | Simulation | chunked_opt_bool_canonical_into[(1000, 10)] |
65.4 µs | 55.1 µs | +18.65% |
| ⚡ | WallTime | threshold_neon[80] |
4.2 µs | 3.6 µs | +18.52% |
| ⚡ | WallTime | filter_neon[64] |
2 µs | 1.8 µs | +16.33% |
| ⚡ |
Simulation | random_i128[0.01] |
6.1 µs | 5.3 µs | +15.18% |
| ⚡ | Simulation | random_i8[0.01] |
117 µs | 103.3 µs | +13.35% |
| ⚡ | WallTime | threshold_neon[8] |
1.5 µs | 1.4 µs | +11.73% |
| ⚡ | WallTime | words_gather_dispatch_neon[65536] |
2 µs | 1.8 µs | +10.38% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ji/extension-order-aggregates (b055982) with develop (cb39bb9)
Footnotes
-
12 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. ↩
IsSorted orders nulls before values, so a null after a value is unsorted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
The adapter is an opt-in pushdown of one aggregate into one extension type's storage, not an aggregate over any extension type, so name it for that and move it to aggregate_fn/storage_pushdown.rs. Expand its docs: registering a pair is an unchecked claim about the extension type; a pushdown is correct only when every operation the aggregate uses (comparison, equality, addition, NaN detection) derives from the same operation on the storage; Timestamp supports it for comparison but not addition. Switch the tests and examples from Sum to MinMax, which is correct for Timestamp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com> # Conflicts: # vortex-array/src/aggregate_fn/storage_pushdown.rs
|
CodSpeed Performance Analysis is red here, and I don't think this PR causes it. The only runtime change is six more entries in the default aggregate session's registry, plus deleting an unreachable The regression count also moves without a code change. a1d0943 flagged 29 benchmarks, and b09c7ed, which only changes test code, flags 18. Most of the benchmarks also appear on #10566 and other PRs. CodSpeed's report itself warns about walltime runs on hosted runners and about benchmarks that spend significant time in system calls. No fix is needed in this PR. The regressions can be acknowledged on CodSpeed. Generated by Claude Code |
…d none count, null_count, all_null, all_non_null and uncompressed_size_in_bytes already accept any extension dtype, so say they need no pushdown instead of listing them as always-correct pushdowns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
…wn, tighten docs Name the extension in the type, and cut its docs down to the two conditions a registration must meet: the extension type's domain supports the aggregation, and the storage computes it soundly, with the operation each aggregate needs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: connortsui20 <87130162+connortsui20@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
Order fixed-size lists of non-nullable u8 by byte in is_sorted and min_max, which is UUID storage, and register the UUID pushdowns in vortex_uuid::initialize: UUIDs are stored big-endian, so they compare like their storage bytes. Accumulate constant extension batches straight into the storage aggregate instead of through a nested accumulator, since a constant storage array has no encoding to dispatch on or stats worth caching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD Signed-off-by: Claude <noreply@anthropic.com>
…-aggregates Signed-off-by: Claude <noreply@anthropic.com> # Conflicts: # vortex-array/src/aggregate_fn/storage_pushdown.rs
|
Is there a way to have more specialized logic for certain UUID extension types? for example, with UUIDv7 we want to only compare the timestamp parts rather than the whole ID lexicographically |
AggregateFnSession::register_storage_ordered(ext)pushes the comparison-based aggregatesis_sortedandmin_maxdown intoext's storage, registeringExtensionStoragePushdown<IsSorted, E>andExtensionStoragePushdown<MinMax, E>.Date,TimeandTimestamp, whose storage is an integer.vortex_uuid::initializecalls it forUuid. UUIDs are stored as 16 big-endian bytes, so byte order is UUID order.is_sortedandmin_maxnow order aFixedSizeListof non-nullableu8(UUID storage) lexicographically by byte. A sharedfns/fixed_size_bytes.rsdecodes the elements once and slices each value. Other fixed-size lists stay unsupported.