fix(reader): resolve sparse Utf8/Binary rows lazily, and stop dropping the fill - #344
Conversation
A `vortex.sparse` Utf8/Binary column whose scanned range holds no patch allocated an (n + 1) entry offsets table of all zeros plus a one-byte value buffer to say "every row is the fill" — 4n bytes for the common case of a genuinely sparse column. `VarBinConstantArray`, added in #329 for exactly this shape, represents it in O(1). Row rendering is unchanged (the empty string per row, or all-null when the fill scalar is null, via the untouched `withSparseValidity`). Closes #340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g the fill SparseEncodingDecoder.decodeVarBin merged every patch into a fresh `length`-row bytes buffer plus an (n + 1) offsets table, walking all n positions to build them. The primitive and bool paths had been lazy since #226/#232; utf8/binary was the one dtype still materialized, for the one encoding whose premise is that most rows are not stored. It also lost the fill. decodeVarBin was handed only `fill.is_valid()`, never the scalar, so unpatched rows were written as zero-length ranges and every one of them read back as the empty string. A sparse utf8 column with a non-null string fill decoded wrong — only its patched rows were right. The Rust `SparseArray` resolves an unpatched row to the fill value whatever the values encoding is, exactly as the primitive path here already did. The existing test used fill "x" but asserted only a patched row, so it never saw this. Root cause was a missing carrier, the same gap #329 closed for vortex.constant and 28cc4f7 for vortex.runend: every primitive type has a LazySparseXxxArray over SparseArrays.findPatch/walkPatches, VarBin had none, so decode had nothing lazy to return. VarBinSparseArray resolves row -> patch (or the fill) on access. bytesSegment()/segmentIfPresent() follow the "no single contiguous buffer" convention of VarBinChunkedArray, VarBinRunEndArray and VarBinConstantArray, so generic consumers still flatten via VarBinArray.toOffsetMode. That also drops a second materialization the old path forced: it called toOffsetMode on the values child, which allocates whenever the pool is view-, dict-, or chunk-backed. Three adversarial tests move from decode time to read time. Two of them (non-monotonic offsets, offsets past the payload) still fail as a VortexException, now from the shared varbin bounds check on the row that uses the bad pair. The third — a patch count beyond what the offsets child covers — no longer raises at all: VarBinEncodingDecoder broadcast-fills a short offsets child, so the read wraps and stays inside the payload. The old exception there was incidental, thrown by overrunning a merge buffer sized from the same broken offsets, not by detecting anything; the test now asserts what ADR 0003 actually requires of that input. Closes #340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d fill Review follow-ups on #340. `offset` was the one field of ProtoPatchesMetadata never range-checked. Every lazy sparse carrier maps logical row `i` to absolute `i + offset` and the sequential walkers iterate `[offset, offset + n)`, so an offset near Long.MAX_VALUE wrapped that end bound negative: forEachByteLength then visited no rows at all while the per-row binary search still resolved every one, two accessors disagreeing on the same array. Checked once next to the patch-count guard, which covers the whole LazySparseXxxArray family since they share SparseArrays.walkPatches. fillBytes conflated "null fill" with "fill carrying the wrong arm of the scalar oneof". An int64 fill on a utf8 column is non-null, so fillValid was true, yet it yielded no bytes and rendered every unpatched row as a valid empty string — the same shape of wrong answer this PR set out to remove. It now fails as a VortexException, matching what ConstantEncodingDecoder does with the same mismatch. Adds the ground-truth interop cover the carrier lacked: a JNI-written (Rust reference) mostly-null utf8 column decodes through VarBinSparseArray, asserted by carrier class and not only by values, since the eager and lazy paths agree on every value here. Probing the pinned compressor could not reach a non-null utf8 fill — mostly-one-value gets vortex.dict, mostly-empty-string gets vortex.fsst over vortex.runend — so that half stays unit-covered, recorded in the test javadoc rather than left as a silent gap. Also: sweeps the offset axis in the walk-vs-search agreement test (the one place they can diverge), lists the two new implementations in VarBinArray's javadoc, corrects the compatibility table's sparse dtype column (decode covers Primitive/Bool/Utf8/Binary, encode Primitive), and states plainly in varBinPatchCountBeyondValueOffsets that the input is a deliberate weakening from "raises" to "wrong but safe". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review pass applied in 2df5f36. Three of the findings were real: Unvalidated
Missing ground-truth cover — added. A JNI-written (Rust reference) mostly-null Utf8 column decodes through Also swept the offset axis in the walk-vs-search agreement test (offsets 0–3, including one that puts a patch behind the window), listed Not taken: extracting the
|
Closes #340.
Started as the allocation fix #340 describes and grew one commit, because the eager merge was also hiding a correctness bug.
The bug
SparseEncodingDecoder.decodeVarBinwas handed onlyfill.is_valid()— a boolean — never the fill scalar itself. Unpatched rows were written into the merged offsets table as zero-length ranges, so every unpatched row of a sparse Utf8/Binary column read back as the empty string instead of the fill. Avortex.sparseutf8 column with a non-null string fill decoded wrong; only its patched rows were ever right.The Rust
SparseArrayresolves an unpatched row to the fill value whatever the values encoding is, exactly as this decoder's own primitive path already did. The existing test used fill"x"and asserted only a patched row, so it never saw it.The change
Two commits:
e692269f— a patch-free range stops allocating an(n + 1)all-zeros offsets table to say "every row is the fill";VarBinConstantArray(added in ConstantEncodingDecoder.decodeString should be lazy, not eager #329) says it in O(1). This is SparseEncodingDecoder allocates an (n+1) offsets table for a zero-patch VarBin column #340 as filed.b72028b7—VarBinSparseArray, the VarBin member of theLazySparseXxxArrayfamily, resolving row → patch (or the fill) on access over the sameSparseArrays.findPatch/walkPatcheshelpers the primitive variants use. The whole merge goes away: thetotalBytespre-pass, the byte copies, the offsets table, thereadVarBinOffset/readUnsignedIdxhelpers, and the boundary try/catch that existed only to cover them. Fixing the dropped fill falls out of it — the carrier holds the fill bytes.Root cause is the same missing-carrier gap #329 closed for
vortex.constantand 28cc4f7 forvortex.runend: every primitive type had a lazy sparse carrier, VarBin had none, so decode had nothing lazy to return.bytesSegment()/segmentIfPresent()follow the "no single contiguous buffer" convention ofVarBinChunkedArray,VarBinRunEndArrayandVarBinConstantArray, so generic consumers still flatten viaVarBinArray.toOffsetMode. That drops a second materialization the old path forced: it calledtoOffsetModeon the values child, which allocates whenever the pool is view-, dict-, or chunk-backed.Adversarial tests
Three move from decode time to read time, which is worth reviewing:
VortexExceptionat decodeVortexExceptionon the row that uses the bad pairVortexExceptionat decodeVortexExceptionon the row that uses the bad pairVortexExceptionat decodeThe third is the one to look at.
VarBinEncodingDecoderbroadcast-fills a short offsets child (i % cap), so the read wraps to a valid pair and stays inside the payload. The old exception was incidental — it came from overrunning a merge buffer that had been sized from the same broken offsets, not from detecting anything. The test now asserts what ADR 0003 actually requires there: no raw JDK exception, no read outside the value buffer. WhetherVarBinEncodingDecodershould reject a truncated (as opposed to single-element/constant) offsets child at all is a separate question, not touched here.Tests
VarBinSparseArrayTest— new, covering accessors, offset rebasing, the fill copy contract,forEachByteLengthvs. per-row search agreement, unsorted indices,toOffsetMode,limited.SparseEncodingDecoderTest— the dropped-fill regression (utf8 and binary fills, since they arrive asstring_valuevs.bytes_value),forEachByteLength, the zero-patch carrier, plus the three adversarial updates.VarBinArrayTest— the zero-length constant corner SparseEncodingDecoder allocates an (n+1) offsets table for a zero-patch VarBin column #340 flagged as possibly uncovered; it was../mvnw verifygreen across all 17 modules, integration included (RustWritesJavaReadsIntegrationTest,NullSparseRunEndInteropIntegrationTest).🤖 Generated with Claude Code