From d6e5d50678400245593decf36423122ee2a843b3 Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Wed, 8 Jul 2026 07:40:54 +0200 Subject: [PATCH] fix: propagate validity in Sparse utf8/binary (VarBin) columns (#232) The same null-loss that #226 fixed for primitive Sparse arrays also hit utf8/binary: decodeVarBin decoded indices via decodeChildSegment (losing the MaskedArray wrapper) and discarded the patch-values' validity mask. Since the Rust ValidityVTable is generic over the values encoding, VarBin must carry the same LazySparseBoolArray row validity. - Move the fill-scalar decode before the dtype branch so fillValid is in scope. - Decode indices as decodeChild (preserving MaskedArray); unwrap to idxData. - Unwrap patch values, capturing patchValidity when it is a MaskedArray. - Use ctx.materialize(idxData) to obtain the raw index segment. - Call withSparseValidity on the result, exactly as the primitive path does. - Add three unit tests: null-fill utf8, non-null fill + null patch, all-valid regression guard. - Add isNullScalar drift-guard comment. Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 1 + .../reader/decode/SparseEncodingDecoder.java | 46 +++++-- .../decode/SparseEncodingDecoderTest.java | 113 +++++++++++++++++- 3 files changed, 146 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 653304ef..36695cff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Null rows no longer silently decode as values: wrapper decoders now propagate the row validity that files from the Python bindings carry deep in the encoding tree. Three representations were dropped — a trailing validity child on `fastlanes.bitpacked` (reached through `vortex.alp`/`vortex.zigzag`/`fastlanes.for`, which delegate validity to their encoded child per the Rust `ValidityChild` contract), dict pools with invalid slots, and dict codes with their own validity — across both the eager `vortex.dict` decoder and the lazy dict layout path. Found on real data: penguins and kepler exported invented values (`32.1`, `0.0`) for thousands of null cells. ([#210](https://github.com/dfa1/vortex-java/issues/210)) - `vortex.runend` propagates nullable run-values' validity: a null run now nulls every row it covers instead of expanding to a filler value (uci-online-retail `customerid` u16? nulls previously decoded as the FoR base). Row validity is a lazy run-end bool over the same run-ends, matching the Rust `ValidityVTable`. ([#225](https://github.com/dfa1/vortex-java/issues/225)) - `vortex.sparse` propagates nullability: a `fill_value: null` array nulls every unpatched position (world-energy-consumption `biofuel_cons_change_pct` f64? previously decoded them as 0.0), and a null patch value nulls its own position. Row validity is a lazy sparse bool whose fill is `fill_value.is_valid()` and whose patch bits are the patch values' validity, matching the Rust `ValidityVTable`. ([#226](https://github.com/dfa1/vortex-java/issues/226)) +- `vortex.sparse` over utf8/binary values now carries the same row validity as the primitive path: a `fill_value: null` string column nulls every unpatched position instead of rendering an empty string, and a nullable patch value nulls its own position instead of surfacing its raw bytes. The Rust `ValidityVTable` is generic over the values encoding, so VarBin reuses the identical sparse-bool validity — completing the #226 fix for string/binary columns. ([#232](https://github.com/dfa1/vortex-java/issues/232)) ### Added diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java index 5e18e807..1fed38a9 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java @@ -60,21 +60,23 @@ public Array decode(DecodeContext ctx) { long n = ctx.rowCount(); - if (ctx.dtype() instanceof DType.Utf8 || ctx.dtype() instanceof DType.Binary) { - return decodeVarBin(ctx, n, numPatches, offset, indicesPtype); - } - // Row validity mirrors the Rust reference `ValidityVTable`: it is a sparse // bool array 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). Dropping either // facet lost nulls: a `fill_value: null` array (world-energy `biofuel_cons_change_pct` // f64?) decoded unpatched rows as 0.0, and a null patch (nuclear_share_energy) - // decoded to raw 0 — #226. + // decoded to raw 0 — #226. The Rust vtable is generic over the values encoding, so + // utf8/binary sparse reuses it verbatim; not doing so lost the same nulls for string + // columns — #232. MemorySegment fillBuf = ctx.buffer(0); ProtoScalarValue fillScalar = decodeFill(fillBuf); boolean fillValid = !isNullScalar(fillScalar); + if (ctx.dtype() instanceof DType.Utf8 || ctx.dtype() instanceof DType.Binary) { + return decodeVarBin(ctx, n, numPatches, offset, indicesPtype, fillValid); + } + if (ctx.dtype() instanceof DType.Bool) { DType indicesDtype = new DType.Primitive(indicesPtype, false); Array patchIndices = ctx.decodeChild(0, indicesDtype, numPatches); @@ -176,8 +178,11 @@ private static ProtoScalarValue decodeFill(MemorySegment fillBuf) { /// Detects a null fill scalar: either an explicit `null_value`, or a scalar with no /// value-bearing field set (Rust encodes a null fill as `ScalarValue::Null`, and a - /// non-null fill always sets exactly one typed field — even integer/float `0`). + /// non-null fill always sets exactly one typed field — even integer/float `0`, or a + /// utf8/binary fill via `string_value`/`bytes_value`). private static boolean isNullScalar(ProtoScalarValue s) { + // keep in sync with ProtoScalarValue components: a new value-bearing field must be + // added below, else a non-null fill of that kind is misclassified as null. return s.null_value() != null || (s.bool_value() == null && s.int64_value() == null && s.uint64_value() == null && s.f32_value() == null && s.f64_value() == null && s.string_value() == null @@ -186,21 +191,35 @@ private static boolean isNullScalar(ProtoScalarValue s) { } private static Array decodeVarBin( - DecodeContext ctx, long n, long numPatches, long offset, PType indicesPtype + DecodeContext ctx, long n, long numPatches, long offset, PType indicesPtype, boolean fillValid ) { + // Patch positions are decoded as an Array (not just a segment) so the shared + // row-validity helper can index them lazily, exactly like the primitive path. + DType indicesDtype = new DType.Primitive(indicesPtype, false); + Array patchIndices = ctx.decodeChild(0, indicesDtype, numPatches); + Array idxData = patchIndices instanceof MaskedArray m ? m.inner() : patchIndices; + MemorySegment outOffsets = ctx.arena().allocate((n + 1) * 4L, 4); if (numPatches == 0) { MemorySegment outBytes = ctx.arena().allocate(1); - return new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32); + Array result = new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32); + return withSparseValidity(ctx, result, fillValid, null, idxData, 0, n, offset); } - DType indicesDtype = new DType.Primitive(indicesPtype, false); - MemorySegment idxSeg = ctx.decodeChildSegment(0, indicesDtype, numPatches); - VarBinArray rawValues = (VarBinArray) ctx.decodeChild(1, ctx.dtype(), numPatches); - VarBinArray.OffsetMode varBin = VarBinArray.toOffsetMode(rawValues, ctx.arena()); + // A nullable patch child arrives wrapped in `vortex.masked`; unwrap it to reach the + // raw VarBin values and carry the per-patch validity bits into the row validity (#232). + Array patchValues = ctx.decodeChild(1, ctx.dtype(), numPatches); + BoolArray patchValidity = null; + Array valData = patchValues; + if (patchValues instanceof MaskedArray m) { + valData = m.inner(); + patchValidity = m.validity(); + } + VarBinArray.OffsetMode varBin = VarBinArray.toOffsetMode((VarBinArray) valData, ctx.arena()); MemorySegment valBytes = varBin.bytesSegment(); MemorySegment valOffsets = varBin.offsetsSegment(); PType valOffPtype = varBin.offsetsPtype(); + MemorySegment idxSeg = ctx.materialize(idxData); int idxBytes = indicesPtype.byteSize(); long totalBytes = 0; @@ -229,7 +248,8 @@ private static Array decodeVarBin( outOffsets.setAtIndex(VortexFormat.LE_INT, pos + 1, (int) bytePos); } - return new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32); + Array result = new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32); + return withSparseValidity(ctx, result, fillValid, patchValidity, idxData, numPatches, n, offset); } private static long readVarBinOffset(MemorySegment seg, long i, PType ptype) { diff --git a/reader/src/test/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoderTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoderTest.java index a4183b9e..933d6048 100644 --- a/reader/src/test/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoderTest.java +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoderTest.java @@ -8,15 +8,18 @@ import io.github.dfa1.vortex.core.proto.ProtoPatchesMetadata; import io.github.dfa1.vortex.core.proto.ProtoScalarValue; import io.github.dfa1.vortex.core.proto.ProtoSparseMetadata; +import io.github.dfa1.vortex.core.proto.ProtoVarBinMetadata; import io.github.dfa1.vortex.encoding.TestSegments; import io.github.dfa1.vortex.reader.ReadRegistry; import io.github.dfa1.vortex.reader.array.Array; import io.github.dfa1.vortex.reader.array.DoubleArray; import io.github.dfa1.vortex.reader.array.MaskedArray; +import io.github.dfa1.vortex.reader.array.VarBinArray; import org.junit.jupiter.api.Test; import java.lang.foreign.Arena; import java.lang.foreign.MemorySegment; +import java.nio.charset.StandardCharsets; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.within; @@ -25,7 +28,8 @@ class SparseEncodingDecoderTest { private static final SparseEncodingDecoder SUT = new SparseEncodingDecoder(); private static final ReadRegistry REGISTRY = TestRegistry.ofDecoders( - SUT, new PrimitiveEncodingDecoder(), new BoolEncodingDecoder()); + SUT, new PrimitiveEncodingDecoder(), new BoolEncodingDecoder(), + new VarBinEncodingDecoder(), new MaskedEncodingDecoder()); @Test void encodingId_isVortexSparse() { @@ -108,6 +112,81 @@ void nonNullFill_validPatches_returnsPlainArray() { assertThat(inner.getDouble(1)).isCloseTo(5.0, within(1e-9)); } + /// The string/binary sibling of #226 (#232): a `fill_value: null` utf8 sparse column must + /// null every UNPATCHED position instead of rendering the empty string. No corpus column hits + /// this shape yet, so this test IS the reproduction — the same world-energy null-fill shape, + /// but with a utf8 values encoding rather than f64. The Rust `ValidityVTable` is generic + /// over the values encoding, so VarBin reuses the same sparse-bool row validity. + @Test + void utf8NullFill_nullsUnpatchedPositions_keepsPatchesValid() { + // Given — null fill; patches "b" at pos 1 and "d" at pos 3 over 5 rows. + MemorySegment[] segs = { + nullFill(), // fill_value: null (type-agnostic) + TestSegments.leInts(1, 3), // patch indices (U32) + utf8Bytes("bd"), // concatenated patch values + TestSegments.leInts(0, 1, 2) // patch value offsets + }; + ArrayNode idxNode = primitiveNode(1); + ArrayNode valNode = varBinNode(2, 3); + + // When + Array result = decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, idxNode, valNode); + + // Then — only the patched positions are valid; patches keep their strings. + MaskedArray masked = assertMasked(result); + assertValidity(masked, false, true, false, true, false); + VarBinArray inner = (VarBinArray) masked.inner(); + assertThat(inner.getBytes(1)).containsExactly('b'); + assertThat(inner.getBytes(3)).containsExactly('d'); + } + + /// A non-null utf8 fill with a null PATCH value must null only that patch's position (#232): + /// previously the dropped mask let a patched-but-null slot render as its raw string bytes. + @Test + void utf8NonNullFill_withNullPatch_nullsOnlyThatPatch() { + // Given — non-null string fill; patches "b" at pos 1 and "d" at pos 3, the patch at pos 3 null. + MemorySegment[] segs = { + utf8Fill("x"), // valid (non-null) string fill + TestSegments.leInts(1, 3), // patch indices + utf8Bytes("bd"), // patch values + TestSegments.leInts(0, 1, 2), // patch value offsets + boolBitmap(true, false) // patch validity: patch 1 (pos 3) null + }; + ArrayNode idxNode = primitiveNode(1); + ArrayNode valNode = maskedVarBinNode(2, 3, 4); + + // When + Array result = decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, idxNode, valNode); + + // Then — fill positions valid, patch at pos 1 valid, patch at pos 3 null. + MaskedArray masked = assertMasked(result); + assertValidity(masked, true, true, true, false, true); + VarBinArray inner = (VarBinArray) masked.inner(); + assertThat(inner.getBytes(1)).containsExactly('b'); + } + + /// No-regression: a non-null utf8 fill with non-nullable patches must NOT produce a [MaskedArray]. + @Test + void utf8NonNullFill_validPatches_returnsPlainArray() { + // Given — non-null string fill, patches all valid. + MemorySegment[] segs = { + utf8Fill("x"), + TestSegments.leInts(1, 3), + utf8Bytes("bd"), + TestSegments.leInts(0, 1, 2) + }; + ArrayNode idxNode = primitiveNode(1); + ArrayNode valNode = varBinNode(2, 3); + + // When + Array result = decode(DType.UTF8, 2, 0, PType.U32, 5, segs, idxNode, valNode); + + // Then + assertThat(result).isInstanceOf(VarBinArray.class).isNotInstanceOf(MaskedArray.class); + VarBinArray inner = (VarBinArray) result; + assertThat(inner.getBytes(1)).containsExactly('b'); + } + private static Array decode(DType dtype, long numPatches, long offset, PType indicesPtype, long n, MemorySegment[] segs, ArrayNode idxNode, ArrayNode valNode) { ProtoPatchesMetadata patches = new ProtoPatchesMetadata( @@ -123,6 +202,10 @@ private static DType nullableF64() { return new DType.Primitive(PType.F64, true); } + private static DType nullableUtf8() { + return new DType.Utf8(true); + } + private static MemorySegment nullFill() { return MemorySegment.ofArray(ProtoScalarValue.ofNullValue(ProtoNullValue.NULL_VALUE).encode()); } @@ -131,6 +214,16 @@ private static MemorySegment f64Fill(double v) { return MemorySegment.ofArray(ProtoScalarValue.ofF64Value(v).encode()); } + /// A non-null utf8 fill scalar: sets `string_value`, so [SparseEncodingDecoder] treats it as + /// valid. Exercises the string-typed field of the fill-null detection helper. + private static MemorySegment utf8Fill(String v) { + return MemorySegment.ofArray(ProtoScalarValue.ofStringValue(v).encode()); + } + + private static MemorySegment utf8Bytes(String s) { + return MemorySegment.ofArray(s.getBytes(StandardCharsets.UTF_8)); + } + private static MaskedArray assertMasked(Array result) { assertThat(result).isInstanceOf(MaskedArray.class); return (MaskedArray) result; @@ -155,6 +248,24 @@ private static ArrayNode maskedPrimitiveNode(int dataBufIndex, int validityBufIn new int[]{dataBufIndex}); } + /// A `vortex.varbin` node: I32 offsets in child 0 (segment `offsetsBufIndex`), byte data in + /// buffer `bytesBufIndex`. + private static ArrayNode varBinNode(int bytesBufIndex, int offsetsBufIndex) { + MemorySegment meta = MemorySegment.ofArray(new ProtoVarBinMetadata(ProtoPType.I32).encode()); + ArrayNode offsets = new ArrayNode(EncodingId.VORTEX_PRIMITIVE, null, new ArrayNode[0], + new int[]{offsetsBufIndex}); + return new ArrayNode(EncodingId.VORTEX_VARBIN, meta, new ArrayNode[]{offsets}, new int[]{bytesBufIndex}); + } + + /// A `vortex.masked` node wrapping a varbin child plus a `vortex.bool` validity child — the + /// shape a nullable varbin patch-values array decodes from. + private static ArrayNode maskedVarBinNode(int bytesBufIndex, int offsetsBufIndex, int validityBufIndex) { + ArrayNode validity = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0], + new int[]{validityBufIndex}); + return new ArrayNode(EncodingId.VORTEX_MASKED, null, + new ArrayNode[]{varBinNode(bytesBufIndex, offsetsBufIndex), validity}, new int[0]); + } + private static MemorySegment boolBitmap(boolean... valid) { byte[] bytes = new byte[(valid.length + 7) / 8]; for (int i = 0; i < valid.length; i++) {