fix: null rows silently decoded as values (three validity representations dropped) + NullArray CSV export#212
Merged
Merged
Conversation
ea53585 to
9469479
Compare
Cross-implementation conformance against the Raincloud corpus: 247 public datasets whose Vortex files are written by the Python bindings (vortex-data 0.69.0), each with a Parquet sibling built from the same Arrow table. - expected-status.csv enumerates every vortex-enabled slug with a triaged status: ok (must match the Parquet oracle exactly), gap:<issue> (must still fail, so a fix forces the flip in the same change), untriaged (runs and reports as aborted without failing the build). - RaincloudConformanceIntegrationTest discovers whatever the hydrate manifest lists; the oracle reads the Parquet sibling through hardwood and writes through the same fastcsv writer as CsvExporter, so a zero-diff proves every value survived the Python-write/Java-read boundary. Oracle-side limitations (nested parquet columns) abort the slug instead of failing it. - scripts/hydrate-raincloud-corpus.sh resolves slugs through the raincloud loader (cache -> mirror -> local build) pinned at v0.2.1, with a per-slug size budget and tolerant per-slug failures. - raincloud-conformance.yml runs a size-capped sweep weekly; off the PR path because upstream dataset URLs rot independently of our code. First triage of 10 hydrated slugs: 4 ok (value-for-value exact), 6 gaps filed as #206 (lazy dict U8/U16 values), #207 (nested struct columns), Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Files from the Python bindings (vortex-data 0.69.0) dict-encode narrow-integer columns; the dict values land as U8/U16 primitives and buildLazyDictPrimitive threw "unsupported ptype for lazy dict". Add DictByteArray and DictShortArray mirroring the existing lazy dict records: raw-signed storage semantics (unsigned widening stays in the ByteArray/ShortArray getInt defaults, keyed on dtype), hoisted code-width switches in materialize/forEach/fold, and the same validateCodes construction guard. F16 remains the only unsupported value ptype. On the Raincloud corpus this flips uci-bike-sharing-dataset to a full value-for-value pass and unblocks the kepler/penguins scans far enough to expose two pre-existing bugs, reclassified in the conformance matrix: nullable ALP columns dropping row validity (#210) and CSV export lacking a NullArray arm (#211). Closes #206 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…xport Files from the Python bindings (vortex-data 0.69.0) carry row validity deep in the encoding tree, in three representations our decoders flattened away — every one silent corruption (nulls exported as invented values), found by the Raincloud conformance suite (#205): - a trailing bool validity child on fastlanes.bitpacked, reached through vortex.alp / vortex.zigzag / fastlanes.for, which per the Rust ValidityChild contract delegate their validity to the encoded child. AlpEncodingDecoder and ZigZagEncodingDecoder now decode that child as an Array (nullability inherited from the parent dtype), split off a MaskedArray, and re-wrap their result — the pattern FrameOfReferenceEncodingDecoder already used. Bitpacked decodes the trailing validity child at the index its patch shape dictates (0 / 2 / 3, mirroring the Rust vtable deserialize). - dict pools with invalid slots: null rows are codes pointing at an invalid pool entry; slot validity is now gathered through the codes into per-row validity. - dict codes with their own validity child: propagated directly, and combined with pool validity when both are present. Applied to the eager vortex.dict decoder (primitive + utf8 paths) and the lazy dict layout path. Also: CSV export renders all-null (DType.Null) columns as empty fields instead of throwing (kepler has fully-empty columns). Corpus evidence: penguins bill_length/bill_depth and 20k+ kepler cells exported values where the file holds nulls (verified against both the Parquet oracle and the Python bindings reading the same file). After the fix kepler passes value-for-value on all 153 columns and flips to ok in the conformance matrix; penguins' remaining failure is #208 only. Closes #210 Closes #211 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…widened Classifications from the round-2 sweep (all verified against the parquet oracle with the fixes on this branch): uci-adult, uci-abalone, uci-mushroom, uci-forest-fires, uci-dry-bean-dataset, insurance, international-football ok; uci-auto-mpg + uci-heart-disease gap:208 (unsigned U8 columns); uci-magic-gamma-telescope gap:215 (new finding: narrow dict-offset ptypes IOOBE in VarBinArray.DictMode). assertStillFails now accepts any RuntimeException as a reproducing gap: a gap may surface as IndexOutOfBoundsException before its decoder gains proper VortexException bounds guards (#215 does exactly that). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
uci-wine, uci-auto-mpg, uci-heart-disease, and the penguins dataset now pass value-for-value with #208 merged beneath this branch — the suite's gap assertions forced the flips (21/21 green over 20 hydrated slugs; matrix now 17 ok / 3 gaps / 227 untriaged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…script Applied from the PR review: assertStillFails now catches only VortexException plus (narrowly, tied to #215) IndexOutOfBoundsException instead of blanket RuntimeException; pool-validity lookups guard out-of-range codes with VortexException per the untrusted-input rule; the hydrate script rejects a valueless --max-mb, misplaced flags, and an empty slug list (bash 3.2 + set -u would otherwise die cryptically); oracle-abort asymmetry documented on oracleLines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9469479 to
bd1639e
Compare
The silent-corruption fix shipped with only the hydration-gated conformance suite covering it. These CI-runnable unit tests pin each validity-propagation seam directly through the decoder harness. - BitpackedEncodingDecoderTest (new): pins validityChildIndex — trailing bool validity at index 0 (no patches), 2 (patches, no chunk offsets), 3 (patches with chunk offsets); no-validity plain array; unexpected child count throws with the expected counts. bit_width=0 constant-residual shape isolates index selection from the packed unpack. - DictEncodingDecoderTest.RowValidity: decodeRustProto's three null representations — pool-null (koi_gmag), codes-null (koi_smet_err2), both (AND semantics), and an out-of-range code guarded as VortexException. - Alp/ZigZag MaskedChildPropagation: masked encoded child surfaces as a MaskedArray with nulls preserved and values decoded (ALP also over the LazyConstant broadcast arm); plain child stays unmasked. - DictLayoutDecoderTest (new): buildLazyDictPrimitive pool/codes gather driven through the public decode() seam with a stub LayoutDecodeContext. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
All review findings addressed:
Full |
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
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.
Fixes the worst finding class of the Raincloud conformance suite (#205): silent null corruption. Files written by the Python bindings (
vortex-data 0.69.0) carry row validity deep in the encoding tree; our decoders flattened it away in three distinct representations, so null rows exported invented values (32.1,0.0, …) with no error.The three representations (all verified against the Rust reference)
fastlanes.bitpacked— reached throughvortex.alp/vortex.zigzag/fastlanes.for, which per Rust'sValidityChildcontract delegate validity to their encoded child.AlpEncodingDecoderandZigZagEncodingDecodernow decode the child as anArray(nullability inherited from the parent dtype), split theMaskedArray, and re-wrap — the patternFrameOfReferenceEncodingDecoderalready used.BitpackedEncodingDecoderreads the trailing validity child at the index its patch shape dictates (0/2/3, mirroring the Rust vtabledeserialize), failing loudly on impossible child counts.vortex.dictdecoder (primitive + utf8 paths) and the lazy dict layout path.Also closes the small one: CSV export renders all-null (
DType.Null) columns as empty fields instead of throwing (#211).Evidence
bill_length_mm/bill_depth_mm: rows 3/271 hold nulls (Python bindings readNone); we exported32.1/13.1. Fixed.okin the conformance matrix../mvnw verifygreen; conformance suite 11/11.Matrix triage after this PR: 6 ok, 4 gaps (#207, #208 ×2, #209), with #208/#209 fixes in flight on separate branches.
Closes #210
Closes #211
🤖 Generated with Claude Code
Note on contents:
maincould not be pushed directly (branch protection), so this branch also carries the two previously-committed pieces this work builds on: the Raincloud conformance harness (79453185, #205) and the lazy-dict U8/U16 fix (a9e41373, closes #206). Squash-merging this PR lands all of it.