fix(writer): three more encoders never reported zone-map MIN/MAX stats - #387
Open
dfa1 wants to merge 4 commits into
Open
fix(writer): three more encoders never reported zone-map MIN/MAX stats#387dfa1 wants to merge 4 commits into
dfa1 wants to merge 4 commits into
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.
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):ConstantEncodingEncoder,RunEndEncodingEncoder, andZigZagEncodingEncoderall hardcodednull, nullfor zone-map stats regardless of input, silently defeatingRowFilterpruning for any column the cascade routed through them. Found via a systematic audit of everywriter/src/main/java/io/github/dfa1/vortex/writer/encode/encoder after fixing #382, grepping for hardcoded null stats on encoders that wrap scalar/comparable data.ConstantEncodingEncoder: min == max == the one repeated value, free to report.RunEndEncodingEncoder: tracks min/max across every run's value (with unsigned comparison for U8/U16/U32/U64) in the existing loop.ZigZagEncodingEncoder: tracks min/max over the original signed values (zigzag's bit-interleaving is not order-preserving, so stats can't be read off the transformed output).Test plan
./mvnw verifygreen across the whole reactorStatsnested class in each*EncodingEncoderTest): reports correct min/max, empty-array stats are null, and (RunEnd) an explicit unsigned-comparison regression case🤖 Generated with Claude Code
https://claude.ai/code/session_01P4ijFsGW1MHEcGiu26vNzi