compute: store accumulable reduce accumulators in columnar form - #38718
compute: store accumulable reduce accumulators in columnar form#38718frankmcsherry wants to merge 2 commits into
Conversation
The accumulable reduce keeps one `Accum` per aggregate in the diff of its input arrangement. `Accum` is an enum sized for its `Numeric` variant, so every slot costs 112 bytes even when it holds a 24 byte `count` or integer `sum`. Deriving `Columnar` for `Accum` and storing the arrangement's diffs in differential's `Coltainer` lays the accumulators out by variant, so each one pays only for its own fields. Supporting changes: `OrderedNumericAgg` in `mz_repr` hosts the `Columnar` impl the orphan rule forbids on `OrderedDecimal<NumericAgg>`; `Overflowing<T>: Columnar` reuses `T`'s container so `Overflowing<i128>` works; `RowLayout`, `RowSpine`, `RowBuilder` and `RowAgent` take a defaulted diff-container parameter; a `HeapSize` trait in `mz_timely_util` lets arrangement size logging cover both container kinds. Measured on 2M keys with count, three integer sums and one float sum, the accumulable arrangement shrinks from 1.21 GB to 436 MB (608 to 221 bytes per record). The batcher still stages `Accum` in columnation chunks, so its `Columnation` impl remains. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
antiguru
left a comment
There was a problem hiding this comment.
I think this looks good. Minor point, which we can gain confidence on by testing, is that we unconditionally switch to the new implementation.
antiguru
left a comment
There was a problem hiding this comment.
Reviewed at 47385cd. Reusing columnar's derive is the right direction here, and the defaulted diff-container parameter on RowLayout/RowSpine/RowBuilder is a clean way to get there without touching other call sites.
Blocking
doctests is the build's only failure. The new doc line at src/row-spine/src/lib.rs:80 links [RowLayout], which is private, and -D rustdoc::private-intra-doc-links rejects it. Unbracketing the reference clears it.
Strong suggestions
No dyncfg gate. This switches the diff container for every accumulable reduce in one release, with no lever short of a revert. Columnar being the destination argues for the direction, not against gating the transition. #38716 wires an equivalent flag through dyncfgs.rs, get_variable_system_parameters, FlipFlagsAction and KNOWN_MISSING_FROM_LD in about fifteen lines, and that also buys CI randomization of both paths.
No test drives Accum through its own container. The two new tests cover the leaves and are well aimed, since Overflowing<i128> and the decimal's raw parts are exactly the two impls written by hand. Nothing exercises the composed enum, Coltainer<Accum> as a BatchContainer, or Semigroup and IsZero agreement across the container boundary. #38716's accums_agree_with_vec_accum is the shape worth borrowing, as it applies seeded random plus_equals and multiply sequences to a reference and compares at every step. Here the reference is Accum itself, so the test is cheap to write.
Coltainer::index reborrows the whole container per read. Differential's src/columnar/layout.rs:101 is self.container.borrow().get(index), and the returned item borrows that temporary, so it cannot be hoisted out of a merge loop. For Accum the borrow materializes thirteen fields across four variants plus the discriminant's two slices and the i128 stores, against a pointer offset for ColumnationStack. This is upstream behavior rather than anything in the change, but Accum is close to the worst shape for it and the cost lands on the merge and consolidation path. The AccumulateReductions feature benchmark has the right plan shape and reports wall clock alongside memory, so a run with and without would close the one axis the measurements do not cover.
mz_arrangement_sizes.allocations changes meaning for this arrangement. Its column comment is "The number of separate memory allocations backing the arrangement", and HeapSize for Coltainer fires one callback per byte slice while log_arrangement_size counts a callback with non-zero capacity as an allocation. The column therefore reports a column count, inflated roughly twentyfold for this container. Reporting size as capacity is defensible, since spare capacity is faulted lazily and serialized storage would make the two equal by definition. The allocation count is a different matter. If columnar exposes no per-allocation walk, the HeapSize doc comment is the right place to say so and name the effect on that column.
Nits
Vec<[u16; NUMERIC_AGG_WIDTH_USIZE]> will not survive the indexed-store path. Twenty-seven u16 is 54 bytes. columnar's FromBytes for &[[T; N]] reads from a word-padded &[u64] through bytemuck::cast_slice, which requires the byte length to divide 54, and its trim term is ((8 - tail) % 8) / 54, which is always zero and so cannot strip the word padding either. from_bytes is unaffected because as_bytes hands over exactly 54 bytes per element, which is why the round-trip test passes, and the i128 store is 16 bytes per element and fine. Nothing calls from_store today, so this is latent, but it sits on the serialized-data path. Widening the column to [u16; 28] makes it 56 bytes with no padding and removes both problems for two bytes per numeric accumulator, and reporting it upstream with a TODO at the type would work too.
from_store and element_sizes are untested in both new impls, for the same reason.
OrderedNumericAgg sits beside OrderedDecimal in mz_repr as a second ordered-decimal type. Orphan rules force it and the doc comment says so. A Deref<Target = NumericAgg> or a From pair would stop call sites reaching for .0.
Reference and owned Ord agree, because Overflowing<T> and OrderedNumericAgg both use the owned value as their reference type, so the derived reference ordering matches field for field. That is load-bearing and invisible, as differential's for<'a> Ref<'a, C>: Ord bound checks that an order exists rather than that it agrees. A future field whose reference type differs from its owned type would break it silently, which is worth a line at the #[columnar(derive(...))] attribute.
Verified, no action needed
- The homogeneous-plan claim holds.
Discriminant::pushkeepsoffset = [tag, count]with an empty variant column while every push shares a variant, so a plan of onlycountand integersumpays sixteen bytes per container rather than nine per element. That is the common shape and the strongest property this layout has over a per-slot tag byte. - Generalizing
Overflowing<T>: Columnarto delegate toT::Containeris the right shape, andOverflowsis named nowhere outsideoverflowing.rs, so dropping itsTC = Vec<T>default breaks no caller. DiffisOverflowing<i64>, whose container still resolves toVec<i64>, so the other arrangements on the columnar batcher paths see no layout change. That is worth stating outright in the description, since it is the reassurance a reviewer of that hunk needs and "primitive cases are unchanged" does not quite give it.
One bound worth keeping visible. The batcher still stages Accum at full width, which is why 64 percent off the arrangement is 21 percent off bulk-hydration RSS. It is already in the follow-ups, and it caps the peak-memory story until the key-only builder that consumes Column chunks lands.
🤖 Posted by Claude Code
Unbracket the `RowLayout` reference in `RowSpine`'s doc, which is a private item and failed the rustdoc lint. Write the `RowSpine` arrangement-size impl over the diff container alone, since the diff type is its `Owned` type. Widen the numeric accumulator's coefficient column from 27 to 28 units so each element is a whole number of `u64` words. columnar's indexed byte store pads columns to words, and its array decoder cannot strip padding whose width does not divide the element width, so the 54 byte column panicked on decode. The round-trip tests now also decode through the indexed store, which fails without the widening. Add a test that drives `Accum` of every variant through its derived container and through `Coltainer<(Vec<Accum>, Diff)>`, checking round trips, byte decoding, and that reference ordering agrees with owned ordering, which the columnar diff container relies on. Note that agreement at the derive, and note what `HeapSize` reports for columnar containers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Motivation
The accumulable reduce (
sum,count,any,all) keeps oneAccumper aggregate in the diff of its input arrangement, as(Vec<Accum>, Diff).Accumis an enum whoseNumericvariant carries a 64 byteDecimal<27>plus four counters, and whose other variants hold ani128, so every slot is 112 bytes regardless of which variant it holds. Acountor integersumneeds 24 of those bytes. Reduces with many groups over integer sums pay for decimals they never use.Description
Accumnow derivesColumnar, and the "ArrangeAccumulable" arrangement stores its diffs in differential'sColtainer<R>, the columnarBatchContainer, instead of a columnation stack. The derived container keeps one set of columns per variant plus a discriminant, so each accumulator occupies only its own variant's fields.Accumitself, itsSemigroup/Multiplyarithmetic, and the output arrangement are unchanged.Supporting pieces, each small:
mz_repr::adt::numeric::OrderedNumericAggis a newtype overNumericAggwithOrderedDecimal's order. The orphan rule blocks aColumnarimpl onOrderedDecimal<NumericAgg>, so this type hosts one. Its container stores the decimal's raw parts as four columns, and its reference type is the owned value so comparisons on references keep decimal order. The coefficient column is one unit wider than the decimal (56 bytes, wholeu64words) because columnar's indexed byte store pads columns to words and cannot decode an element width that does not divide that padding.Overflowing<T>: Columnarinmz_orenow reusesT's own columnar container rather than requiring&[T]to be castable to bytes. That givesOverflowing<i128>, the accumulator count type, aColumnarimpl through columnar's byte-encodedi128store.DiffisOverflowing<i64>, whose container still resolves toVec<i64>, so arrangements on the existing columnar batcher paths see no layout change.RowLayout,RowSpine,RowBuilder(and compute'sRowAgent) gain a defaulted diff-container type parameter. The default is the existing columnation stack, so no other call site changes.mz_timely_util::containers::HeapSizereports heap allocations for a batch container, implemented forColumnationStackandColtainer, and theRowSpinearrangement size logging is generic over the diff container. Each columnar column is oneVec, so the allocation count stays meaningful; columnar exposes no spare capacity, so the reported capacity for columnar diffs is a lower bound.The batcher still stages updates in columnation chunks before they reach the arrangement, so
Accumkeeps itsColumnationimpl and its full width in that transient stage.Measurements
Local,
bin/environmentd --optimized, one worker. A table of 2M rows with 2M distinct keys, indexed asSELECT k, count(*), sum(int4), sum(int8), sum(int4), sum(float8) ... GROUP BY k.mz_arrangement_sizesforArrangeAccumulable [val: empty], which is where the diffs live:The 387 bytes saved per record are five 112 byte slots becoming four 24 byte
SimpleNumberpayloads and one 48 byteFloatpayload, plus the per-element discriminant this mixed-variant plan needs. TheReduceAccumulableoutput arrangement is byte-identical in both runs. Plans whose aggregates are all one variant, for example onlycountand integersum, pay no per-element discriminant at all.clusterd RSS after loading was 21% lower for bulk hydration (2.44 GB vs 3.10 GB) and 8% lower for 20 incremental inserts of 100k rows (3.09 GB vs 3.35 GB). RSS understates the arrangement saving here because the batcher's transient copies are the same on both builds and the macOS system allocator holds on to that peak.
Tests
mz_ore:test_columnar_i128_round_trippushesOverflowing<i128>extremes through the container and back through bytes.mz_repr:ordered_numeric_agg_columnar_round_tripdoes the same for decimals including NaN, infinities, and values at both precision extremes, and also decodes through columnar's indexed store, which fails without the widened coefficient column.mz_compute:accum_columnar_round_trippushes accumulators of every variant, in zero, accumulated, and negated states, throughAccum's derived container and throughColtainer<(Vec<Accum>, Diff)>as aBatchContainer, checking round trips, that reference ordering agrees with owned ordering on every pair, and byte and indexed-store decoding.aggregates.slt,numeric.slt,float.slt,reduce_mfp.slt) exercises the new arrangement end to end.Follow-ups, not in this PR
Rowbuilder that consumesColumnchunks (the key/value spines already haveRowRowColPagedBuilder) would let the batcher stageAccumcolumnar too and drop itsColumnationimpl.u64offset per element when variants are mixed; a fixed-arity diff container, or narrower offsets upstream, would remove that.Coltainer::with_capacityignores its size hint, so the builder path grows the diff columns by doubling; areserveoncolumnar::Containerwould fix this upstream.RowVal/RowRow/ValRowlayouts could take the same diff-container parameter when a consumer appears.No user-visible behavior change.
🤖 Generated with Claude Code