fix: propagate validity in Sparse utf8/binary (VarBin) columns (#232)

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<RunEnd>`. ([#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<Sparse>`. ([#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<Sparse>` 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
 

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<Sparse>`: 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) {

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<Sparse>` 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++) {