fix(writer): thirteen encoders never reported zone-map MIN/MAX stats - #387
Merged
Merged
Conversation
encode() built its EncodeResult via EncodeResult.simple(...), which defaults stats to null regardless of the constant value being encoded -- for a constant array min == max == that value by construction, no scan needed, so any column the cascade collapsed to vortex.constant lost zone-map pruning for free. Fixes #384.
encode() hardcoded (null, null) regardless of the run values actually encoded. RunEnd is specifically favored for clustered, low-cardinality data -- exactly the shape where zone-map pruning otherwise pays off most -- so this was losing the biggest win for its best-fit workload. Tracks min/max across every run's value in the loop that already builds them, with unsigned comparison for U8/U16/U32/U64 (a raw bit pattern that looks negative signed can be a huge unsigned magnitude). Fixes #385.
encode() hardcoded (null, null) regardless of the signed input. Stats must come from the original values, not the zigzag-transformed output -- the bit-interleaving is not order-preserving (e.g. -1 maps to 1, 1 maps to 2), so tracking min/max over the transformed output would have been wrong even if present. Tracks min/max over the original values in the same per-PType switch that already computes the zigzag mapping. Fixes #386.
Same bug, three encoders -- one line naming all three plus their issue numbers reads better than three near-duplicate paragraphs.
EncodeResult.of(root, buffers, stats) and CascadeStep.open(...,
stats) take a {min, max}-or-null pair directly (the shape
PrimitiveEncodingEncoder#minMaxStats / VarBinEncodingEncoder#minMaxStats
already return) instead of making every call site repeat
"stats != null ? stats[0] : null, stats != null ? stats[1] : null".
Every encode path hardcoded (null, null). An arithmetic sequence is monotonic (or constant) end to end, so its extremes are always the first and last element -- free to report from the base/multiplier already computed to validate the sequence, no extra scan needed.
encode() hardcoded (null, null) regardless of input. Computed via PrimitiveEncodingEncoder#minMaxStats over the original typed array, independent of Pco's internal latent/ANS transform.
The numeric encode() path hardcoded (null, null) regardless of input (encodeBool and the empty path correctly stay null: Bool has no zone-map min/max, matching ZoneMapStatCodec#zoneMinMaxDtype). Computed via PrimitiveEncodingEncoder#minMaxStats over the original typed array, independent of RLE's own value/index/offset transform.
Both the terminal encode() and the cascade-open encodeCascade() path hardcoded (null, null) for the numeric fill=0 path regardless of input (encodeBool correctly stays null: Bool has no zone-map min/max). Computed via PrimitiveEncodingEncoder#minMaxStats over the original dense array, which already includes both the fill positions and the patches, so no fill+patch reconstruction is needed.
Both encode() and encodeCascade() hardcoded (null, null) regardless of input. Computed via PrimitiveEncodingEncoder#minMaxStats over the original typed array, independent of which values get patched out.
All four encode paths (primitive/varbin x nullable/non-nullable) hardcoded (null, null) regardless of input. Primitive paths use PrimitiveEncodingEncoder#minMaxStats (nullable case first compacted to only valid elements via PrimitiveArrays#compact, matching MaskedEncodingEncoder's #381 fix -- the dense values array carries placeholder garbage at invalid positions). Utf8 paths use VarBinEncodingEncoder#minMaxStats (which already skips nulls itself); Binary stays unmapped, matching VarBin's own "not usefully zone-mapped" convention for binary blobs.
Both encode() and encodeCascade() hardcoded (null, null) regardless of input. Utf8 columns now report lexicographic min/max via VarBinEncodingEncoder#minMaxStats over the original String[], before symbol-table compression; Binary stays unmapped (blobs aren't usefully zone-mapped).
…tats encode() hardcoded (null, null) regardless of input. Utf8 columns now report lexicographic min/max via VarBinEncodingEncoder#minMaxStats; Binary stays unmapped, matching VarBin's own convention.
…N/MAX stats encode() already propagated the storage encoder's own stats correctly; encodeCascade() hardcoded (null, null) instead, because CascadingCompressor#spliceResult takes a step's stats verbatim and never derives them from a resolved open child -- an Extension column the cascade opens (rather than encode()'s direct path) silently lost its stats. Computed independently over the original data via PrimitiveEncodingEncoder#minMaxStats when the storage dtype is Primitive, matching ZoneMapStatCodec#zoneMinMaxDtype's own rule that an Extension's zone-map min/max is its storage primitive's, unwrapped.
…X stats Both encode() and encodeCascade() hardcoded (null, null) regardless of input. Computed via PrimitiveEncodingEncoder#minMaxStats over the original i64 timestamp array, before it's split into days/seconds/ subseconds -- signed i64 order matches chronological order regardless of the split, so no part-level reconstruction is needed.
Ten of these bugs shipped invisibly because the whole test suite (round-trip property tests, Rust-interop tests) only ever checks decoded VALUES, never stats presence -- broken pruning changes nothing about what a scan returns, only how much it reads, so no existing test could catch it. ZoneMapStatsCoverageTest closes the gap two ways: a registry-driven check that every default-registered encoder accepting a Primitive/Extension/Utf8 dtype has a coverage case (add a new stats-eligible encoder without a case here and it fails), plus the actual per-encoder assertion that encode() reports stats for representative non-empty input.
dfa1
changed the title
4 tasks
dfa1
added a commit
that referenced
this pull request
Sep 13, 2026
…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.
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
Same failure mode as #382 (
AlpRdEncodingEncoder, already merged): thirteen more encoders hardcodednull, nullfor zone-map stats regardless of input, silently defeatingRowFilterpruning for any column they encoded. Found via a systematic audit of everywriter/src/main/java/io/github/dfa1/vortex/writer/encode/encoder after fixing #382.Fixed, one commit each:
ConstantEncodingEncoder— min == max == the one repeated value, free to report.RunEndEncodingEncoder— tracks min/max across every run's value (unsigned-aware for U8/U16/U32/U64).ZigZagEncodingEncoder— tracks min/max over the original signed values (zigzag's bit-interleaving isn't order-preserving).SequenceEncodingEncoder— an arithmetic sequence's extremes are always its first/last element, free to report.PcoEncodingEncoder,RleEncodingEncoder,SparseEncodingEncoder,PatchedEncodingEncoder— computed viaPrimitiveEncodingEncoder#minMaxStatsover the original typed array, independent of each encoder's own internal transform.ZstdEncodingEncoder— all four paths (primitive/varbin x nullable/non-nullable); nullable primitive compacts to valid-only elements first (matching fix(writer,reader): stop zone-map pruning from silently disabling itself #381'sMaskedEncodingEncoderfix).FsstEncodingEncoder,VarBinViewEncodingEncoder— lexicographic string min/max viaVarBinEncodingEncoder#minMaxStats; Binary stays unmapped.ExtEncodingEncoder#encodeCascade— its non-cascadeencode()was already correct; the cascade path lost stats becauseCascadingCompressor#spliceResulttakes a step's stats verbatim and never derives them from a resolved open child.DateTimePartsEncodingEncoder— computed over the original pre-split i64 timestamp, not any of the three split parts.Also:
EncodeResult.of(root, buffers, stats)/CascadeStep.open(root, buffers, children, stats)factories taking a{min, max}-or-nullpair directly, replacing the repeatedstats != null ? stats[0] : null, stats != null ? stats[1] : nullat every call site.ZoneMapStatsCoverageTest: a fitness function asserting every default-registered encoder that accepts aPrimitive/Extension/Utf8dtype has a coverage case reporting real stats. A future encoder missing this now fails a test instead of shipping invisibly -- the whole existing test suite (round-trip property tests, Rust-interop tests) only ever checks decoded values, never stats presence, since broken pruning changes nothing about what a scan returns, only how much it reads.Test plan
./mvnw verifygreen across the whole reactor (twice, before and after the follow-up ten encoders)Statsnested class in each*EncodingEncoderTest) plus the newZoneMapStatsCoverageTestvortex-demo): apricefilter entirely outside the column's domain went from fetching ~54% of the file to ~0.3%🤖 Generated with Claude Code
https://claude.ai/code/session_01P4ijFsGW1MHEcGiu26vNzi