Repository navigation
fix: compute is_sorted for extension (temporal) arrays - #10246
joseph-isaacs wants to merge 2 commits into
Conversation
`IsSorted::return_dtype` and `partial_dtype` rejected every extension dtype, so `is_sorted` and `is_strict_sorted` returned `false` for sorted Timestamp, Date and Time arrays without computing anything, and `check_extension_sorted` was unreachable. Extension values compare by their storage values, so support an extension dtype whenever its storage dtype is supported. The result is computed through the aggregate partial (which now carries extension boundary values across chunks) and cached on the array's statistics. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AVc4YuqkjNi2dU6AaFVaUr Signed-off-by: Claude <[email protected]>
Whether an extension type sorts like its storage values is a property of the extension type, so do not infer it from the storage dtype. Extension types have no encoding of their own, so add an aggregate kernel registry keyed by extension type to `AggregateFnSession` (`register_extension_aggregate_kernel` / `find_extension_aggregate_kernel`), consulted by the accumulator for extension-typed batches. Add `StorageAggregateKernel<A>`, a generic kernel that computes aggregate `A` over the storage values of an extension-typed batch. Registering it for an extension type is the opt-in, with no per-property method on the extension vtable. The default session registers it for `IsSorted` on Date, Time and Timestamp. `is_sorted` keeps storage boundary values in its partial for extension input, returns `false` for an extension type without a registered kernel, and refuses to accumulate one directly. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AVc4YuqkjNi2dU6AaFVaUr Signed-off-by: Claude <[email protected]>
f793047 to
cd880d7
Compare
Merging this PR will improve performance by 54.13%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | density_sweep_single_slice[0.9] |
50.8 µs | 33 µs | +54.13% |
| Simulation | bench_compare_sliced_dict_primitive[(3333, 10000)] |
79.2 µs | < 1 ns | N/A |
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/temporal-is-sorted (cd880d7) with develop (9a08b82)
Footnotes
-
503 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. ↩
|
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 |
No commit since 5fc5cb8 fixed a finding, so U1 to U8 are all still needed. Part 4 now gives line numbers at 1d09649. U5 also covers Time, whose Span setters panic like those of Timestamp. U2 fixes the DataFusion literal conversion of EXT-B1 in the same change, and checks the dynamic filter path. U6 makes the claim in the unpack_native docs true. EXT-A1 records #10566 and #10616, which add composite aggregates on storage for the temporal types and Uuid, while plain is_sorted and min_max keep their old answers. #10246 closed without a merge. The Duration and Interval PRs touch the code of U1, U2 and phase 3. Signed-off-by: Connor Tsui <[email protected]>
Summary
is_sorted/is_strict_sortedalways returnedfalsefor extension arrays, including sorted Timestamp, Date and Time columns.IsSorted::return_dtypeandpartial_dtyperejected everyDType::Extension, sois_sorted_implbailed out early withfalse. As a resultStat::IsSorted/Stat::IsStrictSortedcould never betruefor event-time columns.Whether an extension type sorts like its storage values is a property of the extension type, so it is not inferred from the storage dtype. It is defined per extension type by registering a kernel on the session, so no per-property method is added to
ExtVTable.Changes
AggregateFnSessiongets a kernel registry keyed by extension type (register_extension_aggregate_kernel/find_extension_aggregate_kernel), since extension types have no encoding of their own. The accumulator consults it for extension-typed batches, after encoding kernels andtry_accumulate.StorageAggregateKernel<A>: a generic kernel that computes aggregateAover the storage values of an extension-typed batch. Registering it for an extension type is the opt-in. It declines whenA's partial differs between the extension and storage dtypes.StorageAggregateKernel::<IsSorted>for Date, Time and Timestamp.IsSorted: the partial keeps storage boundary values for extension input, so chunk boundaries are checked.is_sortedreturnsfalsefor an extension type without a registered kernel, and the vtable refuses to accumulate one directly. The removedcheck_extension_sortedassumed storage order for every extension type.DivisibleIntis not sorted until its session registers the storage kernel, and is sorted after.API Changes
AggregateFnSession::register_extension_aggregate_kernelandfind_extension_aggregate_kernel.vortex_array::aggregate_fn::kernels::StorageAggregateKernel<A>.make_is_sorted_partial_dtypeuses the storage dtype for extension input.Checks run:
cargo test -p vortex-array --lib(3931 passed),cargo clippy -p vortex-array --all-targets --all-features -- -D warnings,cargo +nightly-2026-09-10 fmt -p vortex-array.🤖 Generated with Claude Code
https://claude.ai/code/session_01AVc4YuqkjNi2dU6AaFVaUr