Skip to content

Commit d6e5d50

Browse files
dfa1claude
dfa1
and
claude
committed
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<Sparse> 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 <noreply@anthropic.com>
1 parent 512cfbd commit d6e5d50

3 files changed

Lines changed: 146 additions & 14 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2323
- 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))
2424
- `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))
2525
- `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))
26+
- `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))
2627

2728
### Added
2829

‎reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -60,21 +60,23 @@ public Array decode(DecodeContext ctx) {
6060

6161
long n = ctx.rowCount();
6262

63-
if (ctx.dtype() instanceof DType.Utf8 || ctx.dtype() instanceof DType.Binary) {
64-
return decodeVarBin(ctx, n, numPatches, offset, indicesPtype);
65-
}
66-
6763
// Row validity mirrors the Rust reference `ValidityVTable<Sparse>`: it is a sparse
6864
// bool array whose fill is `fill_value.is_valid()` and whose per-patch value is the
6965
// patch value's validity bit. So a position is valid iff (it is a patch AND that
7066
// patch is valid) OR (it is unpatched AND the fill is non-null). Dropping either
7167
// facet lost nulls: a `fill_value: null` array (world-energy `biofuel_cons_change_pct`
7268
// f64?) decoded unpatched rows as 0.0, and a null patch (nuclear_share_energy)
73-
// decoded to raw 0 — #226.
69+
// decoded to raw 0 — #226. The Rust vtable is generic over the values encoding, so
70+
// utf8/binary sparse reuses it verbatim; not doing so lost the same nulls for string
71+
// columns — #232.
7472
MemorySegment fillBuf = ctx.buffer(0);
7573
ProtoScalarValue fillScalar = decodeFill(fillBuf);
7674
boolean fillValid = !isNullScalar(fillScalar);
7775

76+
if (ctx.dtype() instanceof DType.Utf8 || ctx.dtype() instanceof DType.Binary) {
77+
return decodeVarBin(ctx, n, numPatches, offset, indicesPtype, fillValid);
78+
}
79+
7880
if (ctx.dtype() instanceof DType.Bool) {
7981
DType indicesDtype = new DType.Primitive(indicesPtype, false);
8082
Array patchIndices = ctx.decodeChild(0, indicesDtype, numPatches);
@@ -176,8 +178,11 @@ private static ProtoScalarValue decodeFill(MemorySegment fillBuf) {
176178

177179
/// Detects a null fill scalar: either an explicit `null_value`, or a scalar with no
178180
/// value-bearing field set (Rust encodes a null fill as `ScalarValue::Null`, and a
179-
/// non-null fill always sets exactly one typed field — even integer/float `0`).
181+
/// non-null fill always sets exactly one typed field — even integer/float `0`, or a
182+
/// utf8/binary fill via `string_value`/`bytes_value`).
180183
private static boolean isNullScalar(ProtoScalarValue s) {
184+
// keep in sync with ProtoScalarValue components: a new value-bearing field must be
185+
// added below, else a non-null fill of that kind is misclassified as null.
181186
return s.null_value() != null
182187
|| (s.bool_value() == null && s.int64_value() == null && s.uint64_value() == null
183188
&& s.f32_value() == null && s.f64_value() == null && s.string_value() == null
@@ -186,21 +191,35 @@ private static boolean isNullScalar(ProtoScalarValue s) {
186191
}
187192

188193
private static Array decodeVarBin(
189-
DecodeContext ctx, long n, long numPatches, long offset, PType indicesPtype
194+
DecodeContext ctx, long n, long numPatches, long offset, PType indicesPtype, boolean fillValid
190195
) {
196+
// Patch positions are decoded as an Array (not just a segment) so the shared
197+
// row-validity helper can index them lazily, exactly like the primitive path.
198+
DType indicesDtype = new DType.Primitive(indicesPtype, false);
199+
Array patchIndices = ctx.decodeChild(0, indicesDtype, numPatches);
200+
Array idxData = patchIndices instanceof MaskedArray m ? m.inner() : patchIndices;
201+
191202
MemorySegment outOffsets = ctx.arena().allocate((n + 1) * 4L, 4);
192203
if (numPatches == 0) {
193204
MemorySegment outBytes = ctx.arena().allocate(1);
194-
return new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32);
205+
Array result = new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32);
206+
return withSparseValidity(ctx, result, fillValid, null, idxData, 0, n, offset);
195207
}
196208

197-
DType indicesDtype = new DType.Primitive(indicesPtype, false);
198-
MemorySegment idxSeg = ctx.decodeChildSegment(0, indicesDtype, numPatches);
199-
VarBinArray rawValues = (VarBinArray) ctx.decodeChild(1, ctx.dtype(), numPatches);
200-
VarBinArray.OffsetMode varBin = VarBinArray.toOffsetMode(rawValues, ctx.arena());
209+
// A nullable patch child arrives wrapped in `vortex.masked`; unwrap it to reach the
210+
// raw VarBin values and carry the per-patch validity bits into the row validity (#232).
211+
Array patchValues = ctx.decodeChild(1, ctx.dtype(), numPatches);
212+
BoolArray patchValidity = null;
213+
Array valData = patchValues;
214+
if (patchValues instanceof MaskedArray m) {
215+
valData = m.inner();
216+
patchValidity = m.validity();
217+
}
218+
VarBinArray.OffsetMode varBin = VarBinArray.toOffsetMode((VarBinArray) valData, ctx.arena());
201219
MemorySegment valBytes = varBin.bytesSegment();
202220
MemorySegment valOffsets = varBin.offsetsSegment();
203221
PType valOffPtype = varBin.offsetsPtype();
222+
MemorySegment idxSeg = ctx.materialize(idxData);
204223

205224
int idxBytes = indicesPtype.byteSize();
206225
long totalBytes = 0;
@@ -229,7 +248,8 @@ private static Array decodeVarBin(
229248
outOffsets.setAtIndex(VortexFormat.LE_INT, pos + 1, (int) bytePos);
230249
}
231250

232-
return new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32);
251+
Array result = new VarBinArray.OffsetMode(ctx.dtype(), n, outBytes, outOffsets, PType.I32);
252+
return withSparseValidity(ctx, result, fillValid, patchValidity, idxData, numPatches, n, offset);
233253
}
234254

235255
private static long readVarBinOffset(MemorySegment seg, long i, PType ptype) {

‎reader/src/test/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoderTest.java‎

Lines changed: 112 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,18 @@
88
import io.github.dfa1.vortex.core.proto.ProtoPatchesMetadata;
99
import io.github.dfa1.vortex.core.proto.ProtoScalarValue;
1010
import io.github.dfa1.vortex.core.proto.ProtoSparseMetadata;
11+
import io.github.dfa1.vortex.core.proto.ProtoVarBinMetadata;
1112
import io.github.dfa1.vortex.encoding.TestSegments;
1213
import io.github.dfa1.vortex.reader.ReadRegistry;
1314
import io.github.dfa1.vortex.reader.array.Array;
1415
import io.github.dfa1.vortex.reader.array.DoubleArray;
1516
import io.github.dfa1.vortex.reader.array.MaskedArray;
17+
import io.github.dfa1.vortex.reader.array.VarBinArray;
1618
import org.junit.jupiter.api.Test;
1719

1820
import java.lang.foreign.Arena;
1921
import java.lang.foreign.MemorySegment;
22+
import java.nio.charset.StandardCharsets;
2023

2124
import static org.assertj.core.api.Assertions.assertThat;
2225
import static org.assertj.core.api.Assertions.within;
@@ -25,7 +28,8 @@ class SparseEncodingDecoderTest {
2528

2629
private static final SparseEncodingDecoder SUT = new SparseEncodingDecoder();
2730
private static final ReadRegistry REGISTRY = TestRegistry.ofDecoders(
28-
SUT, new PrimitiveEncodingDecoder(), new BoolEncodingDecoder());
31+
SUT, new PrimitiveEncodingDecoder(), new BoolEncodingDecoder(),
32+
new VarBinEncodingDecoder(), new MaskedEncodingDecoder());
2933

3034
@Test
3135
void encodingId_isVortexSparse() {
@@ -108,6 +112,81 @@ void nonNullFill_validPatches_returnsPlainArray() {
108112
assertThat(inner.getDouble(1)).isCloseTo(5.0, within(1e-9));
109113
}
110114

115+
/// The string/binary sibling of #226 (#232): a `fill_value: null` utf8 sparse column must
116+
/// null every UNPATCHED position instead of rendering the empty string. No corpus column hits
117+
/// this shape yet, so this test IS the reproduction — the same world-energy null-fill shape,
118+
/// but with a utf8 values encoding rather than f64. The Rust `ValidityVTable<Sparse>` is generic
119+
/// over the values encoding, so VarBin reuses the same sparse-bool row validity.
120+
@Test
121+
void utf8NullFill_nullsUnpatchedPositions_keepsPatchesValid() {
122+
// Given — null fill; patches "b" at pos 1 and "d" at pos 3 over 5 rows.
123+
MemorySegment[] segs = {
124+
nullFill(), // fill_value: null (type-agnostic)
125+
TestSegments.leInts(1, 3), // patch indices (U32)
126+
utf8Bytes("bd"), // concatenated patch values
127+
TestSegments.leInts(0, 1, 2) // patch value offsets
128+
};
129+
ArrayNode idxNode = primitiveNode(1);
130+
ArrayNode valNode = varBinNode(2, 3);
131+
132+
// When
133+
Array result = decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, idxNode, valNode);
134+
135+
// Then — only the patched positions are valid; patches keep their strings.
136+
MaskedArray masked = assertMasked(result);
137+
assertValidity(masked, false, true, false, true, false);
138+
VarBinArray inner = (VarBinArray) masked.inner();
139+
assertThat(inner.getBytes(1)).containsExactly('b');
140+
assertThat(inner.getBytes(3)).containsExactly('d');
141+
}
142+
143+
/// A non-null utf8 fill with a null PATCH value must null only that patch's position (#232):
144+
/// previously the dropped mask let a patched-but-null slot render as its raw string bytes.
145+
@Test
146+
void utf8NonNullFill_withNullPatch_nullsOnlyThatPatch() {
147+
// Given — non-null string fill; patches "b" at pos 1 and "d" at pos 3, the patch at pos 3 null.
148+
MemorySegment[] segs = {
149+
utf8Fill("x"), // valid (non-null) string fill
150+
TestSegments.leInts(1, 3), // patch indices
151+
utf8Bytes("bd"), // patch values
152+
TestSegments.leInts(0, 1, 2), // patch value offsets
153+
boolBitmap(true, false) // patch validity: patch 1 (pos 3) null
154+
};
155+
ArrayNode idxNode = primitiveNode(1);
156+
ArrayNode valNode = maskedVarBinNode(2, 3, 4);
157+
158+
// When
159+
Array result = decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, idxNode, valNode);
160+
161+
// Then — fill positions valid, patch at pos 1 valid, patch at pos 3 null.
162+
MaskedArray masked = assertMasked(result);
163+
assertValidity(masked, true, true, true, false, true);
164+
VarBinArray inner = (VarBinArray) masked.inner();
165+
assertThat(inner.getBytes(1)).containsExactly('b');
166+
}
167+
168+
/// No-regression: a non-null utf8 fill with non-nullable patches must NOT produce a [MaskedArray].
169+
@Test
170+
void utf8NonNullFill_validPatches_returnsPlainArray() {
171+
// Given — non-null string fill, patches all valid.
172+
MemorySegment[] segs = {
173+
utf8Fill("x"),
174+
TestSegments.leInts(1, 3),
175+
utf8Bytes("bd"),
176+
TestSegments.leInts(0, 1, 2)
177+
};
178+
ArrayNode idxNode = primitiveNode(1);
179+
ArrayNode valNode = varBinNode(2, 3);
180+
181+
// When
182+
Array result = decode(DType.UTF8, 2, 0, PType.U32, 5, segs, idxNode, valNode);
183+
184+
// Then
185+
assertThat(result).isInstanceOf(VarBinArray.class).isNotInstanceOf(MaskedArray.class);
186+
VarBinArray inner = (VarBinArray) result;
187+
assertThat(inner.getBytes(1)).containsExactly('b');
188+
}
189+
111190
private static Array decode(DType dtype, long numPatches, long offset, PType indicesPtype, long n,
112191
MemorySegment[] segs, ArrayNode idxNode, ArrayNode valNode) {
113192
ProtoPatchesMetadata patches = new ProtoPatchesMetadata(
@@ -123,6 +202,10 @@ private static DType nullableF64() {
123202
return new DType.Primitive(PType.F64, true);
124203
}
125204

205+
private static DType nullableUtf8() {
206+
return new DType.Utf8(true);
207+
}
208+
126209
private static MemorySegment nullFill() {
127210
return MemorySegment.ofArray(ProtoScalarValue.ofNullValue(ProtoNullValue.NULL_VALUE).encode());
128211
}
@@ -131,6 +214,16 @@ private static MemorySegment f64Fill(double v) {
131214
return MemorySegment.ofArray(ProtoScalarValue.ofF64Value(v).encode());
132215
}
133216

217+
/// A non-null utf8 fill scalar: sets `string_value`, so [SparseEncodingDecoder] treats it as
218+
/// valid. Exercises the string-typed field of the fill-null detection helper.
219+
private static MemorySegment utf8Fill(String v) {
220+
return MemorySegment.ofArray(ProtoScalarValue.ofStringValue(v).encode());
221+
}
222+
223+
private static MemorySegment utf8Bytes(String s) {
224+
return MemorySegment.ofArray(s.getBytes(StandardCharsets.UTF_8));
225+
}
226+
134227
private static MaskedArray assertMasked(Array result) {
135228
assertThat(result).isInstanceOf(MaskedArray.class);
136229
return (MaskedArray) result;
@@ -155,6 +248,24 @@ private static ArrayNode maskedPrimitiveNode(int dataBufIndex, int validityBufIn
155248
new int[]{dataBufIndex});
156249
}
157250

251+
/// A `vortex.varbin` node: I32 offsets in child 0 (segment `offsetsBufIndex`), byte data in
252+
/// buffer `bytesBufIndex`.
253+
private static ArrayNode varBinNode(int bytesBufIndex, int offsetsBufIndex) {
254+
MemorySegment meta = MemorySegment.ofArray(new ProtoVarBinMetadata(ProtoPType.I32).encode());
255+
ArrayNode offsets = new ArrayNode(EncodingId.VORTEX_PRIMITIVE, null, new ArrayNode[0],
256+
new int[]{offsetsBufIndex});
257+
return new ArrayNode(EncodingId.VORTEX_VARBIN, meta, new ArrayNode[]{offsets}, new int[]{bytesBufIndex});
258+
}
259+
260+
/// A `vortex.masked` node wrapping a varbin child plus a `vortex.bool` validity child — the
261+
/// shape a nullable varbin patch-values array decodes from.
262+
private static ArrayNode maskedVarBinNode(int bytesBufIndex, int offsetsBufIndex, int validityBufIndex) {
263+
ArrayNode validity = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0],
264+
new int[]{validityBufIndex});
265+
return new ArrayNode(EncodingId.VORTEX_MASKED, null,
266+
new ArrayNode[]{varBinNode(bytesBufIndex, offsetsBufIndex), validity}, new int[0]);
267+
}
268+
158269
private static MemorySegment boolBitmap(boolean... valid) {
159270
byte[] bytes = new byte[(valid.length + 7) / 8];
160271
for (int i = 0; i < valid.length; i++) {

0 commit comments

Comments
 (0)