Repository navigation
Compute Widen aggregates on the values - #10890
Conversation
Merging this PR will regress 3 benchmarks3 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
A Narrow aggregate kernel can now run the aggregate on the child and widen its partial, for the integer aggregates where decoding gives the same result. The NR3 brief notes the design that landed. Signed-off-by: Connor Tsui <[email protected]>
| return Ok(None); | ||
| }; | ||
|
|
||
| // NB: Exactness rests on integer decoding. A decimal `Narrow` (CD4 in #10733) needs its own |
There was a problem hiding this comment.
this is a weird comment to leave
There was a problem hiding this comment.
It's because I need to make this change as soon as this stack merges. I can remove it but Im going to change it very soon
robert3005
left a comment
There was a problem hiding this comment.
Not sure where the ls
82532e4 to
0635333
Compare
Narrow aggregates on the valuesWiden aggregates on the values
0635333 to
e978e6f
Compare
e978e6f to
15b34ec
Compare
Move the private `sum_state_scalar` of `SumV2` to `SumState::to_scalar`, so that `Sum`, `SumV2` and encoding kernels build the sum scalar the same way. Document the contracts of `accumulate_primitive` and `AggregateFnVTable::accumulate`. No behavior changes. Signed-off-by: Connor Tsui <[email protected]>
`Mask::last` computed `len - 1` eagerly inside `then_some`, so an empty all-true mask panicked with a subtraction overflow in debug builds. The `Last` aggregate reaches it for every empty batch with all-valid validity. Signed-off-by: Connor Tsui <[email protected]>
Add aggregate kernels for `Widen` that build the exact partial of the logical dtype from the values, so that `MinMax`, `Min`, `Max`, `IsConstant`, `IsSorted` and `Sum` with `skip_nans` no longer widen the values first. The extrema and verdict kernels ask the values and decode any value they return. The sum kernel sums the values into the sum state of the logical dtype. It declines values that have a sum kernel of their own, exact or fallback (`RunEnd`, `Sparse`, `Chunked`), constant values and batches of more than 2^32 rows. Every other aggregate keeps the default path. The kernels are registered in `AggregateFnSession::default` for their exact aggregate ids. Scalar access and the kernels share one decode function. Signed-off-by: Connor Tsui <[email protected]>
Accumulate each aggregate over batches encoded as `Widen` and as canonical arrays, and check every kernel partial, running partial and result. Each consumer builds its own batch, so no accumulator reads a statistic that encoding or another accumulator cached. The cases cover the child bounds, nullable and non-nullable logical dtypes, batch boundaries, empty and all-null batches, a single null under strict `IsSorted`, a running sum overflow, and the sum declining constant and chunked values. Add a `vortex-fastlanes` integration test with a `BitPacked` child. Signed-off-by: Connor Tsui <[email protected]>
Replace the MinMax, IsConstant, IsSorted and Sum kernels of Widen with one kernel that runs the same aggregate on the values and casts the partial to the partial dtype of the logical dtype. Decoding is strictly increasing, one-to-one and keeps validity, so the partials agree, and the values keep their own cached statistics and aggregate kernels. Sum is served only when the aggregate has the Stat::Sum slot, so that each batch is summed from zero, and when the batch has at most 2^32 rows, so that the per-block and per-value overflow checks agree. Signed-off-by: Connor Tsui <[email protected]>
The aggregate kernel no longer decodes scalars, so scalar access is the only caller. Move the function beside it in the operations vtable. Signed-off-by: Connor Tsui <[email protected]>
The Widen aggregate kernel runs Sum through its accumulator and no longer builds a sum state itself. Restore SumV2's private sum_state_scalar, the Sum to_scalar match and the doc lines that only the old Widen sum kernel needed. Signed-off-by: Connor Tsui <[email protected]>
State the Sum overflow bound for 64-bit logical dtypes only, explain that skip_nans matters for integer sums only because it selects the Stat::Sum slot, and say why the chunked values test compares results. Signed-off-by: Connor Tsui <[email protected]>
Signed-off-by: Connor Tsui <[email protected]>
Signed-off-by: Connor Tsui <[email protected]>
A logical type narrower than 64 bits is summed in the same blocks as its values, so its overflow checks agree at any length. Signed-off-by: Connor Tsui <[email protected]>
Sum adds every integer type into a 64-bit sum, so values of b bits cannot overflow it within 2^(64 - b) rows, whatever the logical width. One rule replaces the fixed 2^32 bound and the special case for 64-bit logical types. Signed-off-by: Connor Tsui <[email protected]>
15b34ec to
7dd6ba4
Compare
Summary
Widenencoding for integers and decimals #10733Every aggregate on a
Widencurrently widens the values to the logical type and then runs the canonical accumulator. This PR runsMinMax,Min,Max,IsConstant,IsSortedandSumon the narrow values instead, so they read the values without a widened copy and keep the values' own cached statistics and kernels.Decoding a
Widenvalue is a strictly increasing, one-to-one map that keeps validity, so these aggregates give the same answer on the values as on theWiden, and every value a partial holds widens exactly. One kernel runs the aggregate on the values and casts its partial to the logical dtype. Because the values run the same accumulator code as a canonical array, edge cases such as an empty batch or a single null match by construction. The kernel serves only integer logical types and only these aggregates, which are registered by their exact ids.Based on #10888.
Changes
Sumneeds two more conditions. The accumulator must sum each batch from zero and add the total to the running sum, which it does only forSumwith the legacyStat::Sumslot (Sumwithskip_nans).SumV2andSumwithoutskip_nansadd each value to the running sum, and a batch total cannot match that near the 64-bit bounds. The sum must also be unable to overflow:Sumadds every integer type into a 64-bit sum but checks for overflow at different points for different widths, so the kernel serves only batches of at most 2^(64 − b) rows for values ofbbits, where no check can fire.Mask::lastno longer panics on an empty all-true mask, which the new tests reach throughLaston an empty batch.The tests accumulate each aggregate over batches encoded as
Widenand as canonical arrays, and compare every kernel partial, running partial and final result. Avortex-fastlanesintegration test covers aBitPackedchild.