Skip to content

fix: propagate validity in RunEnd and Sparse lazy decoders (#225, #226)#231

Merged
dfa1 merged 1 commit into
mainfrom
fix/runend-sparse-validity
Jul 7, 2026
Merged

fix: propagate validity in RunEnd and Sparse lazy decoders (#225, #226)#231
dfa1 merged 1 commit into
mainfrom
fix/runend-sparse-validity

Conversation

@dfa1

@dfa1 dfa1 commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Fixes the two round-4 silent-null-loss bugs (#205 triage), same class as the fixed #210: the RunEnd and Sparse lazy decoders unwrapped their children's MaskedArray to .inner() and dropped validity, so null rows decoded as plausible values.

Semantics translated directly from the Rust ValidityVTable impls (encodings/runend/src/array.rs, encodings/sparse/src/lib.rs):

  • RunEnd (RunEndEncodingDecoder drops nullable run-values validity — silent null loss #225): a row's validity is the validity of the run it falls in → row validity is a RunEnd bool over the same ends whose per-run value is the run-value's validity bit. Represented lazily (LazyRunEndBoolArray), O(runs) — essential since n can be huge (uci-online-retail 499,712 rows); a per-row bitmap would defeat the lazy path.
  • Sparse (SparseEncodingDecoder drops nullability — null fill coerced to 0, nullable patch values stripped #226): a position is valid iff (it's a patch AND that patch is valid) OR (it's unpatched AND the fill is non-null). Represented lazily (LazySparseBoolArray), O(patches). Two facets fixed: the null fill value (previously scalarToLong coerced null→0, so unpatched positions rendered 0.0), now detected from the ProtoScalarValue itself (a real 0 fill sets a typed field; null sets null_value/none); and the stripped nullable patch_values mask.

Validation: new RunEndEncodingDecoderTest (3) + SparseEncodingDecoderTest (4) covering null-run/null-fill/null-patch and the no-regression non-nullable path; 2 SparseEncodingEncoderTest round-trips updated (they asserted the old null-fill-as-zero behavior). Real corpus: uci-online-retail (4.3M cells) and world-energy-consumption (3.0M cells) both match the Parquet oracle exactly. Matrix: both gap:225/gap:226ok (0 gaps). Full ./mvnw verify green.

Known follow-up (out of scope, filed separately): VarBin/Utf8 sparse (decodeVarBin) still drops null-fill validity — same class, no corpus column hits it yet.

Closes #225
Closes #226

🤖 Generated with Claude Code

Two silent-null-loss bugs surfaced by Raincloud conformance triage round 4,
both the same class as #210 (validity dropped when unwrapping a MaskedArray
child). Fixed by re-wrapping the decoded result in a MaskedArray whose per-row
validity mirrors the Rust reference vtables — expressed lazily, O(runs)/O(patches)
not O(rows), since n can be huge (uci-online-retail has 499712 rows).

RunEnd (#225): a nullable run-value dropped its mask, so a null run expanded to
a filler value (uci-online-retail `customerid` u16?: null rows emitted the FoR
base). Per Rust `ValidityVTable<RunEnd>`, a RunEnd array's validity IS a RunEnd
over the same ends whose per-run value is the run-value's validity bit; a row's
validity is thus the validity of the run it falls in. Implemented as
`LazyRunEndBoolArray(ends, values-validity, offset)`.

Sparse (#226), two facets. Null fill was coerced to 0 (`scalarToLong` returns 0
for a null scalar), so a `fill_value: null` array decoded every unpatched
position as 0.0 (world-energy-consumption `biofuel_cons_change_pct` f64?).
Nullable patch values were stripped, so a patched-but-null position decoded to
raw 0 (nuclear_share_energy). Per Rust `ValidityVTable<Sparse>`, row validity is
a sparse bool whose fill is `fill_value.is_valid()` and whose per-patch value is
the patch value's validity bit — so a position is valid iff (it is a patch AND
that patch is valid) OR (it is unpatched AND the fill is non-null). Null fill is
detected from the ScalarValue (explicit null_value or no value-bearing field set),
not from `scalarToLong`'s lossy 0.

Corpus evidence: both slugs now match their Parquet oracle value-for-value
(uci-online-retail 4335272 cells, world-energy-consumption 3039010 cells);
expected-status.csv flips both gap entries to ok.

Closes #225
Closes #226

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dfa1

dfa1 commented Jul 7, 2026

Copy link
Copy Markdown
Owner Author

Reviewed (vortex-reviewer, built at ref + ran suites): APPROVE, no correctness/memory blocker. Validity propagation verified faithful to the Rust ValidityVTable contract across all edges — all four Sparse fill×patch combinations, RunEnd first/last/length-1 runs + offset, and isNullScalar confirmed complete over all 11 ProtoScalarValue components (integer 0 / 0.0 / false / "" correctly non-null — no false-negative). Lazy O(runs)/O(patches), no-regression branches return bare arrays. Three nits, non-blocking: (1) isNullScalar could drift if the proto gains a field — folding a keep-in-sync guard into the stacked #232 work which touches the same helper; (2) no gate-running interop fixture (only the weekly corpus flip) — filing as a follow-up; (3) harmless discard-after-decode in the RunEnd unsupported-dtype path.

@dfa1 dfa1 merged commit 512cfbd into main Jul 7, 2026
6 checks passed
dfa1 added a commit that referenced this pull request Jul 8, 2026
Round 5 results (tested against origin/main, 512cfbd):
- bi-citymaxcapita: ok — 28.3M cells match (#231 fixed RunEnd null propagation)
- bi-euro2016: gap:235 — DateTimeParts throws on null day cells (#231 exposed #235)
- bi-hatred: ok
- bi-yalelanguages: gap:235 — DateTimeParts throws on null day cells
- covid-world-vaccination-progress: ok
- hotel-booking-demand: ok (date32 temporal offset matches oracle)
- movies: ok
- uci-credit-approval: ok
- uci-diabetes-130-us-hospitals: ok
- uci-online-retail-ii: ok

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant