refactor(writer): centralize zone-map MIN/MAX stats computation (ADR 0025) - #391
Merged
Merged
Conversation
…0025) Thirteen encoders across #382/#384-#387 independently forgot to compute zone-map stats -- not one bad encoder, an architecture with no structural guarantee, closed only by a coverage test that itself has to be remembered for every future encoder. VortexWriter#writeSegment (the one choke point every written segment already passes through) now falls back to a new ZoneMapStatCodec#columnMinMax whenever the winning encoder's own EncodeResult/CascadeStep didn't already supply stats, computed directly from the segment's untouched (dtype, data) -- mirroring columnSum, which already worked this way and never had this bug. Encoders may still report a cheaper override (kept for Constant/Sequence/RunEnd/ZigZag); none are required to anymore. Confirmed against the Rust reference (spiraldb/vortex): it has no per-encoding stats special-casing either -- StatsSet::compute_stat runs one generic min_max reduction over every array's canonical form. The new ComparableValues marker interface (implemented by DateTimePartsData) is the write-side mirror of exactly what Rust's own canonical.rs::decode_to_temporal does for the same encoding: no special-casing in the generic path, just a carrier that knows its own comparable form. ZoneMapStatsCoverageTest (asserting every encoder self-reports) is replaced by ZoneMapStatCodecTest (asserting columnMinMax handles every dtype shape) -- the guarantee moved from a per-encoder enumeration to the actual new choke point.
Pco, Rle, Sparse, Patched, Zstd, Fsst, VarBinView, Ext's cascade path, and DateTimeParts each computed zone-map stats as a second, unfused pass over the same input the generic fallback (ZoneMapStatCodec#columnMinMax, ADR 0025) now covers from VortexWriter#writeSegment -- same cost either way, but duplicated nine times instead of once. Net code reduction, not a new abstraction on top of the old one.
Sequence keeps its O(1) stats override (an arithmetic sequence's extremes are always its first/last element) since ZoneMapStatsCoverageTest, which used to cover this indirectly, was replaced by ZoneMapStatCodecTest in the prior commit -- the four kept overrides (Constant/Sequence/ RunEnd/ZigZag) need their own direct coverage now that the fitness test moved to testing the fallback instead of every encoder.
The same one-liner restated in nine near-identical files is noise, not documentation -- the why (ADR 0025, the commit history) already explains it once, in one place.
Added earlier in this same change to de-duplicate the "stats != null
? stats[0] : null" pattern across nine encoders -- then all nine had
their stats computation removed outright once it moved to the
central fallback, leaving both factories with zero call sites. None
of the four kept overrides (Constant/Sequence/RunEnd/ZigZag) produce
a {min, max} pair either, so they never fit here anyway.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #382/#384-#387: instead of relying on every
EncodingEncoderto remember to compute its own zone-map stats (the exact convention thirteen encoders independently forgot), move the required computation to the one choke point every written segment already passes through --VortexWriter#writeSegment.ZoneMapStatCodec#columnMinMax(dtype, data)-- a generic fallback computed from the segment's original, untouched input, mirroringcolumnSum(which already worked this way and never had this bug).writeSegmentuses the winning encoder's own stats when present, falls back tocolumnMinMaxotherwise. Encoders may still report a cheaper override; none are required to.Constant/Sequence(O(1), already known from validation) andRunEnd/ZigZag(folded into a pass they run anyway).Pco,Rle,Sparse,Patched,Zstd,Fsst,VarBinView,Ext's cascade path,DateTimeParts.ComparableValuesmarker interface (implemented byDateTimePartsData) for carriers whose logical comparable value isn't their literal shape -- validated against the Rust reference, which has noDateTimeParts-specific stats code either: it decodes to canonical form and runs the same generic reduction. Java's writer doesn't need to decode to get there; the carrier already has the pre-split values.ZoneMapStatsCoverageTest(asserting every encoder self-reports) is replaced byZoneMapStatCodecTest(assertingcolumnMinMaxhandles every dtype shape) -- the actual guarantee moved from a per-encoder enumeration to the new choke point itself.Full design rationale, alternatives considered, and the Rust cross-check: ADR 0025.
Test plan
./mvnw verifygreen across the whole reactorZoneMapStatCodecTest: every dtype shape (Primitivesigned/unsigned,Extension+Primitivestorage,Extension+ComparableValues,Utf8, each nullable) plus excluded shapes (Decimal,Bool,Binary, structural)SequenceEncodingEncoderTest.Stats: direct coverage for the kept override now that the fitness test moved🤖 Generated with Claude Code
https://claude.ai/code/session_01P4ijFsGW1MHEcGiu26vNzi