Repository navigation
feat(aggregate): add ExtensionStoragePushdown to push an aggregate into an extension type's storage - #10566
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>
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | bitpack_blocked_compress_avx2 |
6.7 µs | 7.6 µs | -11.34% |
| ❌ | Simulation | take[routing/grouped_chunks/primitive/nonnull/chunks=32/indices=1000] |
385 µs | 429.4 µs | -10.35% |
| ⚡ | Simulation | chunked_opt_bool_canonical_into[(1000, 10)] |
65.4 µs | 55.1 µs | +18.6% |
| ⚡ ℹ️ | Simulation | test_scan_filter[int] |
3.6 ms | 3 ms | +18.36% |
| ⚡ ℹ️ | Simulation | test_repeated_scan_scalar_at[int_str] |
481.3 µs | 408.6 µs | +17.79% |
| ⚡ | WallTime | threshold_avx2[80] |
3 µs | 2.7 µs | +10.57% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ji/stoic-pascal-p9i2a9 (597e2ae) with develop (61ff28d)
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. ↩
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
|
CodSpeed Performance Analysis is red on 3db29a5, but I don't think either regression is this PR's:
This PR only adds No fix is needed in this PR. The regressions can be acknowledged on CodSpeed. Generated by Claude Code |
|
made a small adjustment in #10612 |
…ate (#10612) Co-authored-by: Claude <noreply@anthropic.com> 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>
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
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>
Benchmarks: Compression 📖Commits: PR vortex / vortex-file-compressed / ns (0.988x ➖, 1↑ 0↓)
vortex / vortex-file-compressed / bytes (1.000x ➖, 0↑ 0↓)
vortex / vortex-file-compressed / ratio (0.992x ➖, 1↑ 0↓)
vortex / parquet / ns (1.000x ➖, 0↑ 0↓)
vortex / parquet / bytes (1.000x ➖, 0↑ 0↓)
vortex / arrow-ipc / ns (1.021x ➖, 0↑ 5↓)
vortex / arrow-ipc / bytes (1.000x ➖, 0↑ 0↓)
|
…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>
…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>
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>
Summary
Aggregates that support an extension dtype currently do so by hand: each one matches
Canonical::Extensionand recurses onstorage_array()(min_max, is_constant, is_sorted, uncompressed_size_in_bytes, the zoned bloom filter). Aggregates that don't, such asIsSorted, reject extension dtypes outright, even where running them on the storage is correct (e.g. timestamps, which compare like theiri64storage).This adds an opt-in pushdown: declaring that one aggregate may run on one extension type's storage is a single registration, with no per-aggregate code:
Changes
New
ExtensionStoragePushdown<A: AggregateFnVTable, E: ExtVTable>invortex-array/src/aggregate_fn/storage_pushdown.rs, anAggregateFnVTableof its own:<A>[<E>](e.g.vortex.is_sorted[vortex.timestamp]), built once innew; it never replacesA. Options serialize and deserialize throughA, so a registered pair round-trips by id.E; any other input makesreturn_dtype/partial_dtypereturnNone.A::Partial, and every vtable call forwards toAwith the storage dtype borrowed from the extension dtype, so partial handling allocates nothing (perf(aggregate): hold the storage partial directly in ExtensionAggregate #10612).Accumulator<A>, so kernels and cached stats registered forAon storage encodings still fire. Constant extension batches become constant storage arrays.Aover the storage dtype, with no lifting back to the extension.The doc comment says a registration is an unchecked claim, and is only correct when two things hold:
E's domain supports the aggregation.It uses
Date/Time/Timestampas the example (comparison yes,sumno), and a doctest registers and runsExtensionStoragePushdown<IsSorted, Timestamp>.The existing hand-written storage delegations are left unchanged: they handle every extension type rather than one specific
E.Tests:
pushes_down_into_storageregistersExtensionStoragePushdown<MinMax, Timestamp>, finds it by id, and checks that the result matchesMinMaxover the storage.combines_flushed_partialschecks that merging two flushed partials matches accumulating both batches.rejects_other_extension_typeschecks that aDateinput is rejected.API Changes
Adds the public
vortex_array::aggregate_fn::ExtensionStoragePushdown. Nothing existing changes.🤖 Generated with Claude Code
https://claude.ai/code/session_01Hp7ZGEG8mrip6qkAtK4iWD