From e692269fe3c05b27e7a2786ec65238ed4d70e3a0 Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Thu, 6 Aug 2026 23:18:39 +0200 Subject: [PATCH 1/3] fix(reader): drop the offsets table for a patch-free sparse VarBin range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 1 + docs/compatibility.md | 2 +- .../reader/decode/SparseEncodingDecoder.java | 10 +++-- .../vortex/reader/array/VarBinArrayTest.java | 14 +++++++ .../decode/SparseEncodingDecoderTest.java | 42 +++++++++++++++++++ 5 files changed, 65 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddad979d..10c6bace 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A `vortex.sequence` column no longer materializes `base + i * multiplier` into a full buffer on decode; rows are computed on access, so the encoding allocates nothing regardless of row count — closing an `OutOfMemoryError` risk from a metadata-only encoding whose row count no buffer bounds. ([#335](https://github.com/dfa1/vortex-java/issues/335)) - A primitive `vortex.dict` column decoded through the encoding path no longer expands its codes into an `n * elemSize` buffer; it now returns the same lazy `DictXxxArray` carriers the layout path already used, so a dict column keeps the dictionary's memory benefit however it is reached. ([#336](https://github.com/dfa1/vortex-java/issues/336)) - A `vortex.patched` column with no patches no longer allocates and copies a full duplicate of its inner child; the child is aliased directly when it already covers every row. ([#337](https://github.com/dfa1/vortex-java/issues/337)) +- A sparse Utf8/Binary column (`vortex.sparse`) whose scanned range holds no patch no longer allocates an `(n + 1)` offsets table of all zeros to describe it; the all-fill range is now represented in O(1). ([#340](https://github.com/dfa1/vortex-java/issues/340)) ## [0.13.1] — 2026-08-06 diff --git a/docs/compatibility.md b/docs/compatibility.md index ddd70c4b..1fc3bb1b 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -144,7 +144,7 @@ decoder falls into one of three shapes: | `vortex.alp` | Lazy | Lazy | `LazyAlpXxxArray`; broadcast → `LazyConstantXxxArray`; patched stays Materialized, ADR 0010 + 0015 | | `vortex.alprd` | Lazy | Lazy | `LazyAlpRdDoubleArray`/`LazyAlpRdFloatArray` — left/right + patches on access | | `vortex.dict` | Lazy | Lazy | `DictXxxArray` (numeric) + `VarBinDictArray` (string), ADR 0012 | -| `vortex.sparse` | Lazy | Lazy | `LazySparseXxxArray` (primitive + bool); Utf8/Binary stays Materialized, ADR 0015 | +| `vortex.sparse` | Lazy | Lazy | `LazySparseXxxArray` (primitive + bool); patched Utf8/Binary stays Materialized, a patch-free range → `VarBinConstantArray`, ADR 0015 | | `vortex.sequence` | Lazy | Lazy | `LazySequenceXxxArray`; `base + i * multiplier` per access, no buffer, ADR 0015 | | `vortex.struct` | Zero-copy | Zero-copy | `StructArray` wraps fields | | `vortex.chunked` | Lazy | Lazy | `ChunkedXxxArray` (primitive/Bool) + `VarBinChunkedArray` (Utf8/Binary), ADR 0012 | 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 741f058f..0c9e8652 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 @@ -26,6 +26,7 @@ import io.github.dfa1.vortex.reader.array.MaterializedBoolArray; import io.github.dfa1.vortex.reader.array.ShortArray; import io.github.dfa1.vortex.reader.array.VarBinArray; +import io.github.dfa1.vortex.reader.array.VarBinConstantArray; import io.github.dfa1.vortex.reader.array.VarBinOffsetArray; import java.io.IOException; @@ -267,12 +268,15 @@ private static Array decodeVarBin( Array idxData = patchIndices instanceof MaskedArray m ? m.inner() : patchIndices; checkPatchChild(idxData, numPatches, "indices"); - MemorySegment outOffsets = ctx.arena().allocate((n + 1) * 4L, 4); if (numPatches == 0) { - MemorySegment outBytes = ctx.arena().allocate(1); - Array result = new VarBinOffsetArray(ctx.dtype(), n, outBytes, outOffsets, PType.I32); + // No patch lands in this range, so every row is the fill — the common case for a + // genuinely sparse column. An (n + 1) offsets table of all zeros says exactly that + // and costs 4n bytes; the constant carrier says it in O(1). `withSparseValidity` + // still nulls every row when the fill scalar is null, as before. + Array result = new VarBinConstantArray(ctx.dtype(), n, new byte[0]); return withSparseValidity(ctx, result, fillValid, null, idxData, 0, n, offset); } + MemorySegment outOffsets = ctx.arena().allocate((n + 1) * 4L, 4); // 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). diff --git a/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinArrayTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinArrayTest.java index 5d5b37f4..f01a7522 100644 --- a/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinArrayTest.java +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinArrayTest.java @@ -384,6 +384,20 @@ void getString_outOfBoundsIndex_throws() { assertThatThrownBy(() -> sut.getString(2)).isInstanceOf(IndexOutOfBoundsException.class); } + /// The zero-length constant is the shape `vortex.sparse` produces for a patch-free range + /// (#340) — `n` empty strings. Every accessor must handle an empty backing array rather + /// than assume at least one byte. + @Test + void emptyValue_readsAsEmptyStringOnEveryRow() { + // Given + VarBinArray sut = new VarBinConstantArray(UTF8, 3, new byte[0]); + + // When / Then + assertThat(sut.getString(0)).isEmpty(); + assertThat(sut.getBytes(2)).isEmpty(); + assertThat(sut.getByteLength(1)).isZero(); + } + @Test void limited_returnsShorterConstant() { // Given 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 8b5a0e8d..b8461f33 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 @@ -16,6 +16,7 @@ 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 io.github.dfa1.vortex.reader.array.VarBinConstantArray; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -190,6 +191,47 @@ void utf8NonNullFill_validPatches_returnsPlainArray() { assertThat(inner.getBytes(1)).containsExactly('b'); } + /// A sparse utf8 column whose scanned range holds no patch at all — the common case for a + /// genuinely sparse column — must not pay an `(n + 1)` offsets table of all zeros to say + /// "every row is the fill" (#340). The constant carrier represents it in O(1). + @Test + void utf8ZeroPatches_returnsConstantCarrier() { + // Given — a non-null string fill and no patches over 5 rows. + MemorySegment[] segs = {utf8Fill("x"), empty(), empty(), empty()}; + ArrayNode idxNode = primitiveNode(1); + ArrayNode valNode = varBinNode(2, 3); + + // When + Array result = decode(DType.UTF8, 0, 0, PType.U32, 5, segs, idxNode, valNode); + + // Then — every row renders as before (the empty string), with no buffer behind it. + assertThat(result).isInstanceOf(VarBinConstantArray.class).isNotInstanceOf(MaskedArray.class); + VarBinArray inner = (VarBinArray) result; + assertThat(inner.length()).isEqualTo(5); + assertThat(inner.getString(0)).isEmpty(); + assertThat(inner.getString(4)).isEmpty(); + assertThat(inner.getByteLength(2)).isZero(); + } + + /// The null-fill half of #340: with no patches and a null fill, every row is null. The + /// carrier changed but the row validity must not — this is what keeps the swap behavior + /// preserving rather than merely allocation-free. + @Test + void utf8ZeroPatchesNullFill_nullsEveryRow() { + // Given — a null fill and no patches over 3 rows. + MemorySegment[] segs = {nullFill(), empty(), empty(), empty()}; + ArrayNode idxNode = primitiveNode(1); + ArrayNode valNode = varBinNode(2, 3); + + // When + Array result = decode(nullableUtf8(), 0, 0, PType.U32, 3, segs, idxNode, valNode); + + // Then + MaskedArray masked = assertMasked(result); + assertValidity(masked, false, false, false); + assertThat(masked.inner()).isInstanceOf(VarBinConstantArray.class); + } + /// A sparse node with 3 children must be rejected: the spec requires exactly 2 /// (patch_indices, patch_values). This is the fail-loud guard against future format /// variants that carry chunk_offsets as a 3rd child (#250). From b72028b7c5468fd04ece753bd37a047837e75d62 Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Thu, 6 Aug 2026 23:37:04 +0200 Subject: [PATCH 2/3] fix(reader): resolve sparse Utf8/Binary rows lazily, and stop dropping the fill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 28cc4f74 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 --- CHANGELOG.md | 3 +- docs/compatibility.md | 2 +- .../reader/array/VarBinSparseArray.java | 115 ++++++++ .../reader/decode/SparseEncodingDecoder.java | 107 ++----- .../reader/array/VarBinSparseArrayTest.java | 270 ++++++++++++++++++ .../decode/SparseEncodingDecoderTest.java | 157 ++++++++-- 6 files changed, 541 insertions(+), 113 deletions(-) create mode 100644 reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinSparseArray.java create mode 100644 reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 10c6bace..a4bc84c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A `vortex.sequence` column no longer materializes `base + i * multiplier` into a full buffer on decode; rows are computed on access, so the encoding allocates nothing regardless of row count — closing an `OutOfMemoryError` risk from a metadata-only encoding whose row count no buffer bounds. ([#335](https://github.com/dfa1/vortex-java/issues/335)) - A primitive `vortex.dict` column decoded through the encoding path no longer expands its codes into an `n * elemSize` buffer; it now returns the same lazy `DictXxxArray` carriers the layout path already used, so a dict column keeps the dictionary's memory benefit however it is reached. ([#336](https://github.com/dfa1/vortex-java/issues/336)) - A `vortex.patched` column with no patches no longer allocates and copies a full duplicate of its inner child; the child is aliased directly when it already covers every row. ([#337](https://github.com/dfa1/vortex-java/issues/337)) -- A sparse Utf8/Binary column (`vortex.sparse`) whose scanned range holds no patch no longer allocates an `(n + 1)` offsets table of all zeros to describe it; the all-fill range is now represented in O(1). ([#340](https://github.com/dfa1/vortex-java/issues/340)) +- A sparse Utf8/Binary column (`vortex.sparse`) with a non-null string or binary fill no longer renders every unpatched row as the empty string: the fill value was dropped on the way into the decode, so only the patched rows were ever right. ([#340](https://github.com/dfa1/vortex-java/issues/340)) +- A sparse Utf8/Binary column no longer merges its patches into a fresh `length`-row bytes buffer plus an `(n + 1)` offsets table on decode; rows resolve to the fill or a patch on access, so the column costs its patches instead of its rows. ([#340](https://github.com/dfa1/vortex-java/issues/340)) ## [0.13.1] — 2026-08-06 diff --git a/docs/compatibility.md b/docs/compatibility.md index 1fc3bb1b..be1358e2 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -144,7 +144,7 @@ decoder falls into one of three shapes: | `vortex.alp` | Lazy | Lazy | `LazyAlpXxxArray`; broadcast → `LazyConstantXxxArray`; patched stays Materialized, ADR 0010 + 0015 | | `vortex.alprd` | Lazy | Lazy | `LazyAlpRdDoubleArray`/`LazyAlpRdFloatArray` — left/right + patches on access | | `vortex.dict` | Lazy | Lazy | `DictXxxArray` (numeric) + `VarBinDictArray` (string), ADR 0012 | -| `vortex.sparse` | Lazy | Lazy | `LazySparseXxxArray` (primitive + bool); patched Utf8/Binary stays Materialized, a patch-free range → `VarBinConstantArray`, ADR 0015 | +| `vortex.sparse` | Lazy | Lazy | `LazySparseXxxArray` (primitive + bool) + `VarBinSparseArray` (Utf8/Binary); fill broadcast, patch resolved per access; patch-free range → `VarBinConstantArray`, ADR 0015 | | `vortex.sequence` | Lazy | Lazy | `LazySequenceXxxArray`; `base + i * multiplier` per access, no buffer, ADR 0015 | | `vortex.struct` | Zero-copy | Zero-copy | `StructArray` wraps fields | | `vortex.chunked` | Lazy | Lazy | `ChunkedXxxArray` (primitive/Bool) + `VarBinChunkedArray` (Utf8/Binary), ADR 0012 | diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinSparseArray.java b/reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinSparseArray.java new file mode 100644 index 00000000..fc601db5 --- /dev/null +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinSparseArray.java @@ -0,0 +1,115 @@ +package io.github.dfa1.vortex.reader.array; + +import io.github.dfa1.vortex.core.model.DType; + +import java.lang.foreign.MemorySegment; +import java.nio.charset.StandardCharsets; +import java.util.Objects; +import java.util.Optional; +import java.util.function.IntConsumer; + +/// Lazy Sparse-encoded [VarBinArray]: `getBytes(i) = patchValues[binSearch(i + offset)]`, or +/// `fill` at every unpatched position. +/// +/// The VarBin member of the `LazySparseXxxArray` family, built on the same +/// `findPatch`/`walkPatches` helpers the primitive variants use. Where those broadcast a +/// scalar fill, this one broadcasts the fill's raw bytes, so a sparse column costs +/// `numPatches` values of storage rather than a merged `length`-row bytes buffer plus an +/// `(length + 1)` offsets table. +/// +/// The fill bytes matter beyond allocation: the eager merge this replaces described unpatched +/// rows as zero-length ranges, which rendered every one of them as the empty string no matter +/// what the fill scalar said. The Rust reference resolves an unpatched row to the fill value +/// for utf8/binary exactly as it does for primitives. +/// +/// [#forEachByteLength(IntConsumer)] walks patches in order (one binary search up front, then +/// a per-patch step) so sequential reads are O(numPatches) work plus `length` emissions, not +/// O(length x log(numPatches)). +/// +/// [#bytesSegment()] is the [MemorySegment#NULL] sentinel and [#segmentIfPresent()] is empty: +/// no single contiguous buffer holds the resolved rows, the same convention +/// [VarBinChunkedArray], [VarBinRunEndArray], and [VarBinConstantArray] use. Consumers that +/// need the flat bytes-plus-offsets shape get it on demand from +/// [VarBinArray#toOffsetMode(VarBinArray, java.lang.foreign.SegmentAllocator)]. +/// +/// The `patchIndices` array is typed as [Array] because the indices ptype varies — backed by +/// one of [ByteArray], [ShortArray], [IntArray], [LongArray]. +/// +/// A patch-free array is not represented here but as a [VarBinConstantArray] over the same +/// fill bytes, which resolves in O(1) with no search at all; `patchValues` is therefore always +/// non-null and non-empty. +/// +/// @param dtype logical element type (Utf8 or Binary) +/// @param length total logical row count +/// @param fill raw bytes of the fill scalar, shared by every unpatched row; +/// [#getBytes(long)] clones it per that method's copy contract +/// @param patchValues values for patched positions; length = `numPatches` +/// @param patchIndices sorted absolute positions of patches; length = `numPatches` +/// @param offset starting absolute position; logical row `i` maps to absolute `i + offset` +@SuppressWarnings("java:S6218") // internal data carrier; record components are arrays of immutable primitives or refs that flow through pipelines without ever being compared. +public record VarBinSparseArray(DType dtype, long length, byte[] fill, + VarBinArray patchValues, Array patchIndices, long offset) + implements VarBinArray { + + /// No single contiguous segment backs the resolved rows. + /// + /// @return the [MemorySegment#NULL] sentinel + @Override + public MemorySegment bytesSegment() { + return MemorySegment.NULL; + } + + /// No single contiguous segment backs the resolved rows. + /// + /// @return always empty + @Override + public Optional segmentIfPresent() { + return Optional.empty(); + } + + @Override + public byte[] getBytes(long i) { + int p = patch(i); + return p >= 0 ? patchValues.getBytes(p) : fill.clone(); + } + + @Override + public String getString(long i) { + int p = patch(i); + return p >= 0 ? patchValues.getString(p) : new String(fill, StandardCharsets.UTF_8); + } + + @Override + public int getByteLength(long i) { + int p = patch(i); + return p >= 0 ? patchValues.getByteLength(p) : fill.length; + } + + @Override + public void forEachByteLength(IntConsumer c) { + int fillLen = fill.length; + SparseArrays.walkPatches(patchIndices, patchValues.length(), offset, offset + length, + () -> c.accept(fillLen), + p -> c.accept(patchValues.getByteLength(p))); + } + + /// Zero-copy truncation: only the row count shrinks, since rows are resolved through + /// `patchIndices` on read and trailing patches past the new end simply go unvisited. + /// + /// @param rows number of leading rows to keep + /// @return a length-`rows` view over the same patches + @Override + public VarBinArray limited(long rows) { + return rows >= length ? this + : new VarBinSparseArray(dtype, rows, fill, patchValues, patchIndices, offset); + } + + /// Locates the patch at logical row `i`. + /// + /// @param i zero-based logical row index (must be in `[0, length)`) + /// @return the patch index, or `-1` when the row is unpatched + private int patch(long i) { + Objects.checkIndex(i, length); + return SparseArrays.findPatch(patchIndices, patchValues.length(), i + offset); + } +} 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 0c9e8652..25564166 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 @@ -4,7 +4,6 @@ import io.github.dfa1.vortex.core.model.PType; import io.github.dfa1.vortex.core.error.VortexException; import io.github.dfa1.vortex.core.model.EncodingId; -import io.github.dfa1.vortex.core.io.VortexFormat; import io.github.dfa1.vortex.core.proto.ProtoPatchesMetadata; import io.github.dfa1.vortex.core.proto.ProtoScalarValue; import io.github.dfa1.vortex.core.proto.ProtoSparseMetadata; @@ -27,11 +26,11 @@ import io.github.dfa1.vortex.reader.array.ShortArray; import io.github.dfa1.vortex.reader.array.VarBinArray; import io.github.dfa1.vortex.reader.array.VarBinConstantArray; -import io.github.dfa1.vortex.reader.array.VarBinOffsetArray; +import io.github.dfa1.vortex.reader.array.VarBinSparseArray; import java.io.IOException; import java.lang.foreign.MemorySegment; -import java.lang.foreign.ValueLayout; +import java.nio.charset.StandardCharsets; /// Read-only decoder for `vortex.sparse`. public final class SparseEncodingDecoder implements EncodingDecoder { @@ -97,7 +96,7 @@ public Array decode(DecodeContext ctx) { boolean fillValid = !isNullScalar(fillScalar); if (ctx.dtype() instanceof DType.Utf8 || ctx.dtype() instanceof DType.Binary) { - return decodeVarBin(ctx, n, numPatches, offset, indicesPtype, fillValid); + return decodeVarBin(ctx, n, numPatches, offset, indicesPtype, fillValid, fillScalar); } if (ctx.dtype() instanceof DType.Bool) { @@ -259,7 +258,8 @@ private static boolean isNullScalar(ProtoScalarValue s) { } private static Array decodeVarBin( - DecodeContext ctx, long n, long numPatches, long offset, PType indicesPtype, boolean fillValid + DecodeContext ctx, long n, long numPatches, long offset, PType indicesPtype, + boolean fillValid, ProtoScalarValue fillScalar ) { // 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. @@ -267,16 +267,14 @@ private static Array decodeVarBin( Array patchIndices = ctx.decodeChild(0, indicesDtype, numPatches); Array idxData = patchIndices instanceof MaskedArray m ? m.inner() : patchIndices; checkPatchChild(idxData, numPatches, "indices"); + byte[] fill = fillBytes(fillScalar); if (numPatches == 0) { // No patch lands in this range, so every row is the fill — the common case for a - // genuinely sparse column. An (n + 1) offsets table of all zeros says exactly that - // and costs 4n bytes; the constant carrier says it in O(1). `withSparseValidity` - // still nulls every row when the fill scalar is null, as before. - Array result = new VarBinConstantArray(ctx.dtype(), n, new byte[0]); + // genuinely sparse column, resolved in O(1) with no search (#340). + Array result = new VarBinConstantArray(ctx.dtype(), n, fill); return withSparseValidity(ctx, result, fillValid, null, idxData, 0, n, offset); } - MemorySegment outOffsets = ctx.arena().allocate((n + 1) * 4L, 4); // 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). @@ -287,81 +285,26 @@ private static Array decodeVarBin( valData = m.inner(); patchValidity = m.validity(); } - VarBinOffsetArray varBin = VarBinArray.toOffsetMode( - checkedCast(valData, VarBinArray.class, "values"), ctx.arena()); - MemorySegment valBytes = varBin.bytesSegment(); - MemorySegment valOffsets = varBin.offsetsSegment(); - PType valOffPtype = varBin.offsetsPtype(); - MemorySegment idxSeg = ctx.materialize(idxData); - - int idxBytes = indicesPtype.byteSize(); - MemorySegment outBytes; - // The patch-value offsets and the declared patch count come from untrusted metadata and - // are deliberately not cross-validated up front (VarBin decode stays lazy). Both loops - // below index `valOffsets` at `numPatches + 1` and copy `valBytes` sub-ranges, so an - // over-long patch count or a non-monotonic offsets pair walks off a segment. The guard - // is a boundary catch-and-wrap around the whole merge rather than a per-element range - // test, so the copy loop stays uniform (CLAUDE.md hot-loop rule) — the malformed file - // still fails as a VortexException, never a raw IndexOutOfBoundsException (ADR 0003). - try { - long totalBytes = 0; - for (long i = 0; i < numPatches; i++) { - totalBytes += readVarBinOffset(valOffsets, i + 1, valOffPtype) - - readVarBinOffset(valOffsets, i, valOffPtype); - } - // Patched bytes are a subset of the value buffer, so a total outside it means the - // offsets disagree with the payload; catching it here also keeps the allocation - // below bounded by data that actually exists (no OutOfMemoryError zip bomb). - if (totalBytes < 0 || totalBytes > valBytes.byteSize()) { - throw new VortexException(EncodingId.VORTEX_SPARSE, - "patch bytes " + totalBytes + " out of range for a value buffer of " - + valBytes.byteSize() + " byte(s)"); - } - outBytes = ctx.arena().allocate(Math.max(1, totalBytes)); - long patchCursor = 0; - long bytePos = 0; - for (long pos = 0; pos < n; pos++) { - if (patchCursor < numPatches) { - long patchPos = readUnsignedIdx(idxSeg, SegmentBroadcast.elementOffset(idxSeg, patchCursor, idxBytes), indicesPtype) - offset; - if (patchPos == pos) { - long strStart = readVarBinOffset(valOffsets, patchCursor, valOffPtype); - long strEnd = readVarBinOffset(valOffsets, patchCursor + 1, valOffPtype); - long strLen = strEnd - strStart; - if (strLen > 0) { - MemorySegment.copy(valBytes, strStart, outBytes, bytePos, strLen); - bytePos += strLen; - } - patchCursor++; - } - } - outOffsets.setAtIndex(VortexFormat.LE_INT, pos + 1, (int) bytePos); - } - } catch (IndexOutOfBoundsException e) { - throw new VortexException(EncodingId.VORTEX_SPARSE, - "patch value offsets out of range for " + numPatches + " patch(es) over a " - + valOffsets.byteSize() + "-byte offsets buffer", e); - } - - Array result = new VarBinOffsetArray(ctx.dtype(), n, outBytes, outOffsets, PType.I32); + VarBinArray values = checkedCast(valData, VarBinArray.class, "values"); + Array result = new VarBinSparseArray(ctx.dtype(), n, fill, values, idxData, offset); return withSparseValidity(ctx, result, fillValid, patchValidity, idxData, numPatches, n, offset); } - private static long readVarBinOffset(MemorySegment seg, long i, PType ptype) { - return switch (ptype) { - case I32, U32 -> Integer.toUnsignedLong(seg.getAtIndex(VortexFormat.LE_INT, i)); - case I64, U64 -> seg.getAtIndex(VortexFormat.LE_LONG, i); - default -> throw new VortexException(EncodingId.VORTEX_SPARSE, "unsupported offset ptype " + ptype); - }; - } - - private static long readUnsignedIdx(MemorySegment seg, long off, PType ptype) { - return switch (ptype) { - case U8 -> Byte.toUnsignedLong(seg.get(ValueLayout.JAVA_BYTE, off)); - case U16 -> Short.toUnsignedLong(seg.get(VortexFormat.LE_SHORT, off)); - case U32 -> Integer.toUnsignedLong(seg.get(VortexFormat.LE_INT, off)); - case U64 -> seg.get(VortexFormat.LE_LONG, off); - default -> throw new VortexException(EncodingId.VORTEX_SPARSE, "non-unsigned index ptype " + ptype); - }; + /// Extracts the raw bytes an unpatched utf8/binary row resolves to. A utf8 fill arrives as + /// `string_value`, a binary fill as `bytes_value`; a null fill has neither, and its bytes + /// are never read — [#withSparseValidity] marks every unpatched row invalid — so the empty + /// array stands in. + /// + /// @param fill the decoded fill scalar + /// @return the fill's raw bytes, empty when the fill carries no utf8/binary payload + private static byte[] fillBytes(ProtoScalarValue fill) { + if (fill.string_value() != null) { + return fill.string_value().getBytes(StandardCharsets.UTF_8); + } + if (fill.bytes_value() != null) { + return fill.bytes_value(); + } + return new byte[0]; } private static long scalarToLong(ProtoScalarValue scalar) { diff --git a/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java new file mode 100644 index 00000000..ae7fdb99 --- /dev/null +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java @@ -0,0 +1,270 @@ +package io.github.dfa1.vortex.reader.array; + +import io.github.dfa1.vortex.core.error.VortexException; +import io.github.dfa1.vortex.core.model.DType; +import io.github.dfa1.vortex.core.model.PType; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.lang.foreign.Arena; +import java.lang.foreign.MemorySegment; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class VarBinSparseArrayTest { + + private static final DType UTF8 = DType.UTF8; + + /// Fill `"zz"` with patches `["b", "cccc"]` at absolute positions 1 and 4. The fill and the + /// two patches all have different byte lengths, so a row resolved against the wrong slot is + /// visible in `getByteLength` alone, not only in the bytes. + private static VarBinSparseArray sut(long length, long offset) { + return new VarBinSparseArray(UTF8, length, "zz".getBytes(StandardCharsets.UTF_8), + values("b", "cccc"), indices(1, 4), offset); + } + + @Nested + class Accessors { + + @ParameterizedTest + @CsvSource({"0,zz", "1,b", "2,zz", "3,zz", "4,cccc", "5,zz"}) + void getString_resolvesPatchedRowsToPatchesAndTheRestToTheFill(long row, String expected) { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When + String result = sut.getString(row); + + // Then + assertThat(result).isEqualTo(expected); + } + + @ParameterizedTest + @CsvSource({"0,2", "1,1", "2,2", "4,4", "5,2"}) + void getByteLength_matchesTheResolvedValue(long row, int expected) { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When + int result = sut.getByteLength(row); + + // Then + assertThat(result).isEqualTo(expected); + } + + @Test + void getBytes_returnsThePatchBytes() { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When + byte[] result = sut.getBytes(4); + + // Then + assertThat(result).isEqualTo("cccc".getBytes(StandardCharsets.UTF_8)); + } + + /// [VarBinArray#getBytes(long)]'s contract is a copy per call. The fill is one array + /// shared by every unpatched row, so handing it out directly would let one row's + /// mutation leak into all the others. + @Test + void getBytes_returnsIndependentCopyOfTheFillEachCall() { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When + byte[] first = sut.getBytes(0); + first[0] = (byte) 'Q'; + + // Then + assertThat(sut.getBytes(2)).isEqualTo("zz".getBytes(StandardCharsets.UTF_8)); + } + + @Test + void getString_outOfBoundsIndex_throws() { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When / Then + assertThatThrownBy(() -> sut.getString(6)).isInstanceOf(IndexOutOfBoundsException.class); + } + + /// An empty fill is what a null-fill column carries, since its unpatched rows are + /// invalid and their bytes never observed. The accessors must still answer for them + /// rather than assume at least one byte. + @Test + void emptyFill_unpatchedRowsAreEmpty() { + // Given + VarBinSparseArray sut = new VarBinSparseArray(UTF8, 3, new byte[0], + values("b"), indices(1), 0); + + // When / Then + assertThat(sut.getString(0)).isEmpty(); + assertThat(sut.getByteLength(0)).isZero(); + assertThat(sut.getString(1)).isEqualTo("b"); + } + } + + @Nested + class Offset { + + /// A sliced chunk starts partway into the absolute patch positions: with `offset = 2`, + /// logical row 2 is absolute position 4, the second patch. + @ParameterizedTest + @CsvSource({"0,zz", "1,zz", "2,cccc", "3,zz"}) + void getString_rebasesRowsThroughOffset(long row, String expected) { + // Given + VarBinSparseArray sut = sut(4, 2); + + // When + String result = sut.getString(row); + + // Then + assertThat(result).isEqualTo(expected); + } + } + + @Nested + class Sequential { + + @Test + void forEachByteLength_emitsFillAndPatchLengthsInRowOrder() { + // Given + VarBinSparseArray sut = sut(6, 0); + List lengths = new ArrayList<>(); + + // When + sut.forEachByteLength(lengths::add); + + // Then + assertThat(lengths).containsExactly(2, 1, 2, 2, 4, 2); + } + + /// The walk and the per-row binary search are two independent resolutions of the same + /// data; a disagreement between them is the bug this guards. + @Test + void forEachByteLength_agreesWithGetByteLength() { + // Given + VarBinSparseArray sut = sut(6, 0); + List walked = new ArrayList<>(); + List searched = new ArrayList<>(); + + // When + sut.forEachByteLength(walked::add); + for (long i = 0; i < sut.length(); i++) { + searched.add(sut.getByteLength(i)); + } + + // Then + assertThat(walked).isEqualTo(searched); + } + + /// The patch indices come from an untrusted file and the walk assumes them sorted; + /// unsorted ones must fail as a [VortexException], never a raw JDK exception (ADR 0003). + @Test + void forEachByteLength_unsortedPatchIndices_throws() { + // Given — patches at 4 then 1, going backwards + VarBinSparseArray sut = new VarBinSparseArray(UTF8, 6, "zz".getBytes(StandardCharsets.UTF_8), + values("b", "cccc"), indices(4, 1), 0); + + // When / Then + assertThatThrownBy(() -> sut.forEachByteLength(len -> { })) + .isInstanceOf(VortexException.class) + .hasMessageContaining("not sorted"); + } + } + + @Nested + class Representation { + + /// No single contiguous buffer holds the resolved rows, so generic consumers must be + /// steered to [VarBinArray#toOffsetMode] rather than handed a bytes segment. + @Test + void bytesSegment_isNullAndSegmentIfPresentIsEmpty() { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When / Then + assertThat(sut.bytesSegment()).isEqualTo(MemorySegment.NULL); + assertThat(sut.segmentIfPresent()).isEmpty(); + } + + /// The flattening path every consumer that needs bytes-plus-offsets goes through. + @Test + void toOffsetMode_flattensFillAndPatchesInRowOrder() { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When + VarBinArray result = VarBinArray.toOffsetMode(sut, Arena.ofAuto()); + + // Then + assertThat(strings(result)).containsExactly("zz", "b", "zz", "zz", "cccc", "zz"); + } + + @Test + void limited_keepsPatchesAndShrinksOnlyTheRowCount() { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When + VarBinArray result = sut.limited(3); + + // Then — zero-copy: same fill, patches, indices and offset + assertThat(result).isEqualTo(new VarBinSparseArray(UTF8, 3, sut.fill(), + sut.patchValues(), sut.patchIndices(), 0)); + assertThat(strings(result)).containsExactly("zz", "b", "zz"); + } + + @Test + void limited_atOrAboveLength_returnsSameInstance() { + // Given + VarBinSparseArray sut = sut(6, 0); + + // When + VarBinArray result = sut.limited(6); + + // Then + assertThat(result).isSameAs(sut); + } + } + + private static VarBinOffsetArray values(String... patchValues) { + byte[] allBytes = String.join("", patchValues).getBytes(StandardCharsets.UTF_8); + int[] offs = new int[patchValues.length + 1]; + for (int i = 0; i < patchValues.length; i++) { + offs[i + 1] = offs[i] + patchValues[i].getBytes(StandardCharsets.UTF_8).length; + } + ByteBuffer bb = ByteBuffer.allocate(offs.length * 4).order(ByteOrder.LITTLE_ENDIAN); + for (int o : offs) { + bb.putInt(o); + } + return new VarBinOffsetArray(UTF8, patchValues.length, MemorySegment.ofArray(allBytes), + MemorySegment.ofArray(bb.array()), PType.I32); + } + + private static Array indices(int... positions) { + ByteBuffer bb = ByteBuffer.allocate(positions.length * 4).order(ByteOrder.LITTLE_ENDIAN); + for (int p : positions) { + bb.putInt(p); + } + return new MaterializedIntArray(new DType.Primitive(PType.U32, false), positions.length, + MemorySegment.ofArray(bb.array())); + } + + private static List strings(VarBinArray array) { + List out = new ArrayList<>(); + for (long i = 0; i < array.length(); i++) { + out.add(array.getString(i)); + } + return out; + } +} 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 b8461f33..10d7652f 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 @@ -17,12 +17,15 @@ import io.github.dfa1.vortex.reader.array.MaskedArray; import io.github.dfa1.vortex.reader.array.VarBinArray; import io.github.dfa1.vortex.reader.array.VarBinConstantArray; +import io.github.dfa1.vortex.reader.array.VarBinSparseArray; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import java.lang.foreign.Arena; import java.lang.foreign.MemorySegment; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -191,6 +194,82 @@ void utf8NonNullFill_validPatches_returnsPlainArray() { assertThat(inner.getBytes(1)).containsExactly('b'); } + /// The utf8/binary path resolved every UNPATCHED row to the empty string instead of the + /// fill: the eager merge was handed only `fill.is_valid()` and described unpatched rows as + /// zero-length ranges, so a non-null string fill was silently dropped. The Rust + /// `SparseArray` resolves an unpatched row to the fill value whatever the values encoding + /// is, exactly as the primitive path here already did. + @Test + void utf8NonNullFill_unpatchedRowsRenderTheFill() { + // Given — fill "zz"; patches "b" at pos 1 and "d" at pos 3 over 5 rows. + MemorySegment[] segs = { + utf8Fill("zz"), + 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 — rows 0, 2, 4 are the fill; the patched rows keep their own values. + assertThat(result).isInstanceOf(VarBinSparseArray.class); + VarBinArray inner = (VarBinArray) result; + assertThat(inner.getString(0)).isEqualTo("zz"); + assertThat(inner.getString(1)).isEqualTo("b"); + assertThat(inner.getString(2)).isEqualTo("zz"); + assertThat(inner.getString(3)).isEqualTo("d"); + assertThat(inner.getString(4)).isEqualTo("zz"); + assertThat(inner.getByteLength(0)).isEqualTo(2); + } + + /// The same fill resolution over `forEachByteLength`, which walks patches rather than + /// binary-searching per row — the two paths must agree. + @Test + void utf8NonNullFill_forEachByteLengthEmitsFillLengths() { + // Given — 2-byte fill, 1-byte patches at positions 1 and 3 over 5 rows. + MemorySegment[] segs = { + utf8Fill("zz"), + TestSegments.leInts(1, 3), + utf8Bytes("bd"), + TestSegments.leInts(0, 1, 2) + }; + List lengths = new ArrayList<>(); + + // When + Array result = decode(DType.UTF8, 2, 0, PType.U32, 5, segs, primitiveNode(1), varBinNode(2, 3)); + ((VarBinArray) result).forEachByteLength(lengths::add); + + // Then + assertThat(lengths).containsExactly(2, 1, 2, 1, 2); + } + + /// A binary (not utf8) fill arrives as the scalar's `bytes_value` rather than + /// `string_value`; both must reach the carrier, or binary columns keep the dropped-fill bug + /// after utf8 stops having it. + @Test + void binaryNonNullFill_unpatchedRowsRenderTheFill() { + // Given — a binary fill of 0x01 0x02, one patch "b" at position 1 over 3 rows. + MemorySegment[] segs = { + MemorySegment.ofArray(ProtoScalarValue.ofBytesValue(new byte[]{1, 2}).encode()), + TestSegments.leInts(1), + utf8Bytes("b"), + TestSegments.leInts(0, 1) + }; + + // When + Array result = decode(new DType.Binary(false), 1, 0, PType.U32, 3, segs, + primitiveNode(1), varBinNode(2, 3)); + + // Then + VarBinArray inner = (VarBinArray) result; + assertThat(inner.getBytes(0)).containsExactly(1, 2); + assertThat(inner.getBytes(1)).containsExactly('b'); + assertThat(inner.getBytes(2)).containsExactly(1, 2); + } + /// A sparse utf8 column whose scanned range holds no patch at all — the common case for a /// genuinely sparse column — must not pay an `(n + 1)` offsets table of all zeros to say /// "every row is the fill" (#340). The constant carrier represents it in O(1). @@ -204,13 +283,13 @@ void utf8ZeroPatches_returnsConstantCarrier() { // When Array result = decode(DType.UTF8, 0, 0, PType.U32, 5, segs, idxNode, valNode); - // Then — every row renders as before (the empty string), with no buffer behind it. + // Then — every row is the fill, with no buffer behind it. assertThat(result).isInstanceOf(VarBinConstantArray.class).isNotInstanceOf(MaskedArray.class); VarBinArray inner = (VarBinArray) result; assertThat(inner.length()).isEqualTo(5); - assertThat(inner.getString(0)).isEmpty(); - assertThat(inner.getString(4)).isEmpty(); - assertThat(inner.getByteLength(2)).isZero(); + assertThat(inner.getString(0)).isEqualTo("x"); + assertThat(inner.getString(4)).isEqualTo("x"); + assertThat(inner.getByteLength(2)).isEqualTo(1); } /// The null-fill half of #340: with no patches and a null fill, every row is null. The @@ -358,12 +437,15 @@ void negativePatchCount_throws() { .hasMessageContaining("patch count"); } - /// `IndexOutOfBoundsException` — the utf8/binary path merges patches eagerly and reads - /// `numPatches + 1` entries from the patch-value offsets buffer. Declaring 4 patches over - /// a values child that only carries 2 walked past the end of that buffer. + /// Declaring 4 patches over a values child carrying only 3 offsets is absorbed by the + /// `SegmentBroadcast` convention: [VarBinEncodingDecoder] broadcast-fills a short + /// offsets child, so patch 3 resolves through offset index `3 % 3 = 0` and reads inside + /// the payload. The eager merge used to raise here, but only incidentally — it overran a + /// merge buffer it had itself sized from the same broken offsets. What ADR 0003 requires + /// is what this asserts: no raw JDK exception, and no read outside the value buffer. @Test - void varBinPatchCountBeyondValueOffsets_throws() { - // Given — 4 declared patches but only 2 value offsets pairs ("b", "d") + void varBinPatchCountBeyondValueOffsets_broadcastsWithoutRawException() { + // Given — 4 declared patches but only 3 offsets, covering 2 values ("b", "d") MemorySegment[] segs = { nullFill(), TestSegments.leInts(0, 1, 2, 3), // 4 patch indices, so the count is plausible @@ -371,19 +453,20 @@ void varBinPatchCountBeyondValueOffsets_throws() { TestSegments.leInts(0, 1, 2) // only 3 offsets = 2 values }; - // When / Then - assertThatThrownBy(() -> decode(nullableUtf8(), 4, 0, PType.U32, 5, segs, - primitiveNode(1), varBinNode(2, 3))) - .isInstanceOf(VortexException.class) - .hasMessageContaining("patch value offsets out of range"); + // When — row 3 resolves to patch 3, past what the offsets child covers + VarBinArray result = assertVarBin(decode(nullableUtf8(), 4, 0, PType.U32, 5, segs, + primitiveNode(1), varBinNode(2, 3))); + + // Then — the broadcast wraps to offsets [0, 1), still inside the 2-byte payload + assertThat(result.getString(3)).isEqualTo("b"); } - /// `IndexOutOfBoundsException` from `MemorySegment.copy` — non-monotonic patch-value - /// offsets make the per-patch lengths cancel out, so the output buffer is sized far - /// smaller than the first patch that actually gets copied into it. + /// `IndexOutOfBoundsException` from `MemorySegment.copy` under the eager merge — + /// non-monotonic patch-value offsets give a patch a negative length. Read lazily, the + /// same pair reaches the shared varbin length check on the row that uses it. @Test - void varBinNonMonotonicValueOffsets_throws() { - // Given — offsets 0, 2, 0: patch 0 is 2 bytes long, patch 1 is -2, total 0 bytes + void varBinNonMonotonicValueOffsets_throwsOnRead() { + // Given — offsets 0, 2, 0: patch 0 is 2 bytes long, patch 1 is -2 MemorySegment[] segs = { nullFill(), TestSegments.leInts(0, 1), @@ -391,17 +474,22 @@ void varBinNonMonotonicValueOffsets_throws() { TestSegments.leInts(0, 2, 0) }; - // When / Then - assertThatThrownBy(() -> decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, - primitiveNode(1), varBinNode(2, 3))) + // When — row 1 resolves to patch 1, whose [2, 0) range runs backwards + VarBinArray result = assertVarBin(decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, + primitiveNode(1), varBinNode(2, 3))); + + // Then + assertThatThrownBy(() -> result.getString(1)) .isInstanceOf(VortexException.class) - .hasMessageContaining("patch value offsets out of range"); + .hasMessageContaining("out of range for a data buffer"); } /// The offsets-vs-payload cross-check: a patch-value offsets buffer claiming more bytes - /// than the value buffer holds must be rejected before the output buffer is sized from it. + /// than the value buffer holds must not read past the payload. Nothing is sized from the + /// offsets any more — no buffer is allocated at all — so the guard that matters is the + /// per-row one on the read. @Test - void varBinValueOffsetsBeyondValueBuffer_throws() { + void varBinValueOffsetsBeyondValueBuffer_throwsOnRead() { // Given — 2 bytes of value data but offsets claiming a 1 GiB final patch MemorySegment[] segs = { nullFill(), @@ -410,11 +498,22 @@ void varBinValueOffsetsBeyondValueBuffer_throws() { TestSegments.leInts(0, 1, 1 << 30) }; - // When / Then - assertThatThrownBy(() -> decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, - primitiveNode(1), varBinNode(2, 3))) + // When — row 1 resolves to patch 1, whose end offset is past the 2-byte payload + VarBinArray result = assertVarBin(decode(nullableUtf8(), 2, 0, PType.U32, 5, segs, + primitiveNode(1), varBinNode(2, 3))); + + // Then + assertThatThrownBy(() -> result.getString(1)) .isInstanceOf(VortexException.class) - .hasMessageContaining("patch bytes"); + .hasMessageContaining("out of range for a data buffer"); + } + + /// A null fill wraps the values in a [MaskedArray]; the adversarial reads above target + /// the values array underneath it. + private static VarBinArray assertVarBin(Array result) { + Array inner = result instanceof MaskedArray m ? m.inner() : result; + assertThat(inner).isInstanceOf(VarBinArray.class); + return (VarBinArray) inner; } } From 2df5f366f4aff5ba20ab617f4c94fc786bf0558f Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Fri, 7 Aug 2026 00:01:05 +0200 Subject: [PATCH 3/3] fix(reader): validate the sparse patch offset, fail loud on a mistyped fill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/compatibility.md | 2 +- ...ullSparseRunEndInteropIntegrationTest.java | 69 +++++++++++++++++-- .../dfa1/vortex/reader/array/VarBinArray.java | 6 +- .../reader/decode/SparseEncodingDecoder.java | 33 +++++++-- .../reader/array/VarBinSparseArrayTest.java | 17 +++-- .../decode/SparseEncodingDecoderTest.java | 60 +++++++++++++++- 6 files changed, 165 insertions(+), 22 deletions(-) diff --git a/docs/compatibility.md b/docs/compatibility.md index be1358e2..2161b4f7 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -92,7 +92,7 @@ integer width is wire-legal, mirroring `VarBinArray`), and `ScanIterator` could | `vortex.alp` | `AlpEncodingDecoder` | `AlpEncodingEncoder` | ✅ | ✅ | F64, F32 | | `vortex.alprd` | `AlpRdEncodingDecoder` | `AlpRdEncodingEncoder` | ✅ | ✅ | F64, F32 | | `vortex.dict` | `DictEncodingDecoder` | `DictEncodingEncoder` | ✅ | ✅ | Primitive, Utf8/Binary | -| `vortex.sparse` | `SparseEncodingDecoder` | `SparseEncodingEncoder` | ✅ | ✅ | Primitive | +| `vortex.sparse` | `SparseEncodingDecoder` | `SparseEncodingEncoder` | ✅ | ✅ | Primitive, Bool, Utf8/Binary (decode); Primitive (encode) | | `vortex.sequence` | `SequenceEncodingDecoder` | `SequenceEncodingEncoder` | ✅ | ✅ | Primitive | | `vortex.struct` | `StructEncodingDecoder` | `StructEncodingEncoder` | ✅ | ✅ | Struct | | `vortex.chunked` | `ChunkedEncodingDecoder` | `ChunkedEncodingEncoder` | ✅ | ✅ | Primitive + Struct concat | diff --git a/integration/src/test/java/io/github/dfa1/vortex/integration/NullSparseRunEndInteropIntegrationTest.java b/integration/src/test/java/io/github/dfa1/vortex/integration/NullSparseRunEndInteropIntegrationTest.java index 29f891b2..40bf66fe 100644 --- a/integration/src/test/java/io/github/dfa1/vortex/integration/NullSparseRunEndInteropIntegrationTest.java +++ b/integration/src/test/java/io/github/dfa1/vortex/integration/NullSparseRunEndInteropIntegrationTest.java @@ -14,6 +14,8 @@ import io.github.dfa1.vortex.reader.array.MaskedArray; import io.github.dfa1.vortex.reader.array.NullArray; import io.github.dfa1.vortex.reader.array.ShortArray; +import io.github.dfa1.vortex.reader.array.VarBinArray; +import io.github.dfa1.vortex.reader.array.VarBinSparseArray; import org.apache.arrow.c.ArrowArray; import org.apache.arrow.c.ArrowSchema; import org.apache.arrow.c.Data; @@ -21,6 +23,7 @@ import org.apache.arrow.vector.BigIntVector; import org.apache.arrow.vector.Float8Vector; import org.apache.arrow.vector.SmallIntVector; +import org.apache.arrow.vector.VarCharVector; import org.apache.arrow.vector.VectorSchemaRoot; import org.apache.arrow.vector.types.FloatingPointPrecision; import org.apache.arrow.vector.types.pojo.ArrowType; @@ -30,6 +33,7 @@ import org.junit.jupiter.api.io.TempDir; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.util.ArrayList; import java.util.HashMap; @@ -48,10 +52,11 @@ /// /// The bundled `vortex-jni` compressor is version-pinned, so its encoding choice for a crafted /// input is deterministic — verified by probing: a mostly-null column with a small fraction of -/// scattered non-`ALP`-able values compresses to `vortex.sparse` with a null fill scalar, and a -/// column of distinct value runs interleaved with null runs compresses to `vortex.runend` with -/// null run-values. Both assertions below therefore hard-check the chosen encoding; a future JNI -/// bump that changes the choice is a deliberate change that should refresh this fixture. +/// scattered non-`ALP`-able values compresses to `vortex.sparse` with a null fill scalar (for +/// utf8 too, not only for primitives), and a column of distinct value runs interleaved with null +/// runs compresses to `vortex.runend` with null run-values. Every assertion below therefore +/// hard-checks the chosen encoding; a future JNI bump that changes the choice is a deliberate +/// change that should refresh this fixture. class NullSparseRunEndInteropIntegrationTest { private static final Session SESSION = Session.create(); @@ -119,6 +124,41 @@ void jniNullFillSparse_i64_nullRowsDecodeNull(@TempDir Path tmp) throws IOExcept assertThat(result).containsExactly(expected); } + /// The utf8 sibling of the two null-fill sparse cases above, and the ground-truth cover for + /// the lazy `VarBinSparseArray` carrier (#340) — before it, this shape merged every patch + /// into a full `length`-row bytes buffer, and the fill was dropped on the way in. + /// + /// Only the null-fill half is reachable from here: probing the pinned JNI compressor with a + /// utf8 column that is mostly one NON-null value gets `vortex.dict` (or `vortex.fsst` over + /// `vortex.runend` for a mostly-empty-string column), never sparse with a non-null string + /// fill. So the non-null-fill fix stays unit-covered only until a corpus file or a JNI bump + /// produces that shape. + @Test + void jniNullFillSparse_utf8_nullRowsDecodeNull(@TempDir Path tmp) throws IOException { + // Given — a nullable utf8 column, ~99% null with a few scattered distinct strings. + Schema schema = new Schema(List.of(Field.nullable("v", new ArrowType.Utf8()))); + String[] expected = new String[ROWS]; + Path file = tmp.resolve("null_fill_sparse_utf8.vtx"); + writeJni(file, schema, (root, i) -> { + VarCharVector vec = (VarCharVector) root.getVector("v"); + if (i % 97 == 0) { + String value = "x" + i; + expected[i] = value; + vec.setSafe(i, value.getBytes(StandardCharsets.UTF_8)); + } else { + vec.setNull(i); + } + }); + + // When + List result = readUtf8(file); + + // Then — the column decodes through the lazy sparse carrier, and every row round-trips. + assertThat(usedEncodings(file)).contains("vortex.sparse"); + assertThat(carrier(file)).isEqualTo(VarBinSparseArray.class); + assertThat(result).containsExactly(expected); + } + @Test void jniNullRunRunEnd_i16_nullRowsDecodeNull(@TempDir Path tmp) throws IOException { // Given — the uci-online-retail `customerid` u16? shape: runs of a distinct value per @@ -254,6 +294,27 @@ private static List readI64(Path file) throws IOException { return out; } + private static List readUtf8(Path file) throws IOException { + var out = new ArrayList(); + forEachValue(file, (masked, inner, i) -> out.add(masked.isValid(i) ? ((VarBinArray) inner).getString(i) : null)); + return out; + } + + /// Returns the concrete [Array] class the single column's payload decodes to, so a test can + /// assert the decode path taken and not only the values it produced — the eager and lazy + /// paths agree on every value here, so values alone would pass on both. + /// + /// @param file the Vortex file to scan + /// @return the runtime class of the first chunk's unwrapped payload array + /// @throws IOException if the file cannot be opened or scanned + private static Class carrier(Path file) throws IOException { + try (VortexReader reader = VortexReader.open(file, ReadRegistry.loadAll()); + var iter = reader.scan(ScanOptions.all())) { + MaskedArray masked = iter.next().column("v"); + return masked.inner().getClass(); + } + } + private static List readI16(Path file) throws IOException { var out = new ArrayList(); forEachValue(file, (masked, inner, i) -> out.add(masked.isValid(i) ? ((ShortArray) inner).getShort(i) : null)); diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinArray.java b/reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinArray.java index 0bc96794..1c5fcf7f 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinArray.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/array/VarBinArray.java @@ -14,8 +14,10 @@ /// Implementations: [VarBinOffsetArray] for standard offset-based layout, [VarBinDictArray] for /// dictionary-encoded strings, [VarBinChunkedArray] for multi-chunk columns, [VarBinViewArray] /// for Arrow StringView / BinaryView layout (16-byte view per row + zero or more shared data -/// buffers), [VarBinSlicedArray] for a zero-copy row-shifted view, and [VarBinConstantArray] for -/// a `vortex.constant` value broadcast across every row. All accessors resolve transparently +/// buffers), [VarBinSlicedArray] for a zero-copy row-shifted view, [VarBinConstantArray] for +/// a `vortex.constant` value broadcast across every row, [VarBinRunEndArray] for a +/// `vortex.runend` column resolved run-by-run, and [VarBinSparseArray] for a `vortex.sparse` +/// column resolved patch-by-patch. All accessors resolve transparently /// regardless of implementation; only [VarBinOffsetArray] exposes /// [VarBinOffsetArray#offsetsSegment()] and [VarBinOffsetArray#offsetsPtype()]. /// 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 25564166..681bf891 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 @@ -81,6 +81,15 @@ public Array decode(DecodeContext ctx) { throw new VortexException(EncodingId.VORTEX_SPARSE, "patch count " + numPatches + " out of range for " + n + " row(s)"); } + // Every lazy sparse carrier maps logical row `i` to absolute `i + offset`, and the + // sequential walkers iterate `[offset, offset + n)`. An untrusted offset near + // Long.MAX_VALUE wraps that end bound negative, which makes the walk visit no rows at + // all while the per-row binary search still resolves each one — the two accessors then + // disagree on the same array. A negative offset is likewise not a position. + if (offset < 0 || offset > Long.MAX_VALUE - n) { + throw new VortexException(EncodingId.VORTEX_SPARSE, + "patch offset " + offset + " out of range for " + n + " row(s)"); + } // 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 @@ -267,7 +276,7 @@ private static Array decodeVarBin( Array patchIndices = ctx.decodeChild(0, indicesDtype, numPatches); Array idxData = patchIndices instanceof MaskedArray m ? m.inner() : patchIndices; checkPatchChild(idxData, numPatches, "indices"); - byte[] fill = fillBytes(fillScalar); + byte[] fill = fillBytes(fillScalar, fillValid); if (numPatches == 0) { // No patch lands in this range, so every row is the fill — the common case for a @@ -291,19 +300,29 @@ private static Array decodeVarBin( } /// Extracts the raw bytes an unpatched utf8/binary row resolves to. A utf8 fill arrives as - /// `string_value`, a binary fill as `bytes_value`; a null fill has neither, and its bytes - /// are never read — [#withSparseValidity] marks every unpatched row invalid — so the empty - /// array stands in. + /// `string_value`, a binary fill as `bytes_value`. + /// + /// A null fill has neither, and its bytes are never read — [#withSparseValidity] marks + /// every unpatched row invalid — so the empty array stands in. A fill that is non-null but + /// carries some other arm of the scalar oneof (an integer fill on a utf8 column, say) is a + /// malformed file rather than an empty string: silently rendering every unpatched row as a + /// valid `""` would be the same class of bug this decode path just stopped having. /// - /// @param fill the decoded fill scalar - /// @return the fill's raw bytes, empty when the fill carries no utf8/binary payload - private static byte[] fillBytes(ProtoScalarValue fill) { + /// @param fill the decoded fill scalar + /// @param fillValid `true` when the fill scalar is non-null + /// @return the fill's raw bytes, empty for a null fill + /// @throws VortexException if a non-null fill carries no string or bytes value + private static byte[] fillBytes(ProtoScalarValue fill, boolean fillValid) { if (fill.string_value() != null) { return fill.string_value().getBytes(StandardCharsets.UTF_8); } if (fill.bytes_value() != null) { return fill.bytes_value(); } + if (fillValid) { + throw new VortexException(EncodingId.VORTEX_SPARSE, + "utf8/binary fill scalar carries no string or bytes value"); + } return new byte[0]; } diff --git a/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java index ae7fdb99..370b992c 100644 --- a/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java @@ -81,8 +81,8 @@ void getBytes_returnsIndependentCopyOfTheFillEachCall() { VarBinSparseArray sut = sut(6, 0); // When - byte[] first = sut.getBytes(0); - first[0] = (byte) 'Q'; + byte[] result = sut.getBytes(0); + result[0] = (byte) 'Q'; // Then assertThat(sut.getBytes(2)).isEqualTo("zz".getBytes(StandardCharsets.UTF_8)); @@ -149,11 +149,16 @@ void forEachByteLength_emitsFillAndPatchLengthsInRowOrder() { } /// The walk and the per-row binary search are two independent resolutions of the same - /// data; a disagreement between them is the bug this guards. - @Test - void forEachByteLength_agreesWithGetByteLength() { + /// data; a disagreement between them is the bug this guards. Swept across offsets + /// because rebasing is where the two can diverge — the walk iterates the absolute range + /// `[offset, offset + length)` while the search rebases each row on its own, so an + /// offset that puts a patch before, at, or after the window exercises different code in + /// each. Offset 2 puts the first patch (absolute 1) behind the window entirely. + @ParameterizedTest + @CsvSource({"0", "1", "2", "3"}) + void forEachByteLength_agreesWithGetByteLength(long offset) { // Given - VarBinSparseArray sut = sut(6, 0); + VarBinSparseArray sut = sut(6 - offset, offset); List walked = new ArrayList<>(); List searched = new ArrayList<>(); 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 10d7652f..c89192ce 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 @@ -437,12 +437,18 @@ void negativePatchCount_throws() { .hasMessageContaining("patch count"); } + /// A deliberate weakening: this input used to raise and now returns a wrong-but-safe + /// answer, so the malformed file is no longer reported at all. + /// /// Declaring 4 patches over a values child carrying only 3 offsets is absorbed by the /// `SegmentBroadcast` convention: [VarBinEncodingDecoder] broadcast-fills a short /// offsets child, so patch 3 resolves through offset index `3 % 3 = 0` and reads inside /// the payload. The eager merge used to raise here, but only incidentally — it overran a - /// merge buffer it had itself sized from the same broken offsets. What ADR 0003 requires - /// is what this asserts: no raw JDK exception, and no read outside the value buffer. + /// merge buffer it had itself sized from the same broken offsets, so the throw was a + /// side effect of the merge rather than a check of anything. Detecting it properly + /// belongs in [VarBinEncodingDecoder], which is what decides that a truncated offsets + /// child broadcasts instead of failing; the contract this test can hold the sparse + /// decoder to is ADR 0003's: no raw JDK exception, no read outside the value buffer. @Test void varBinPatchCountBeyondValueOffsets_broadcastsWithoutRawException() { // Given — 4 declared patches but only 3 offsets, covering 2 values ("b", "d") @@ -508,6 +514,56 @@ void varBinValueOffsetsBeyondValueBuffer_throwsOnRead() { .hasMessageContaining("out of range for a data buffer"); } + /// Logical row `i` maps to absolute `i + offset` in every lazy sparse carrier, and the + /// sequential walkers iterate `[offset, offset + n)`. An offset near `Long.MAX_VALUE` + /// wraps that end bound negative, which made `forEachByteLength` visit no rows at all + /// while `getByteLength` still resolved each one — two accessors disagreeing on the same + /// array. `offset` is untrusted proto metadata and was the one field of it never + /// range-checked. + @Test + void offsetNearMaxLong_throws() { + // Given — an offset whose sum with the row count overflows + MemorySegment[] segs = {utf8Fill("zz"), TestSegments.leInts(1), utf8Bytes("b"), + TestSegments.leInts(0, 1)}; + + // When / Then + assertThatThrownBy(() -> decode(DType.UTF8, 1, Long.MAX_VALUE - 2, PType.U32, 6, segs, + primitiveNode(1), varBinNode(2, 3))) + .isInstanceOf(VortexException.class) + .hasMessageContaining("patch offset"); + } + + @Test + void negativeOffset_throws() { + // Given — a negative absolute start position + MemorySegment[] segs = {utf8Fill("zz"), TestSegments.leInts(1), utf8Bytes("b"), + TestSegments.leInts(0, 1)}; + + // When / Then + assertThatThrownBy(() -> decode(DType.UTF8, 1, -1, PType.U32, 6, segs, + primitiveNode(1), varBinNode(2, 3))) + .isInstanceOf(VortexException.class) + .hasMessageContaining("patch offset"); + } + + /// A fill that is non-null but carries the wrong arm of the scalar oneof — an integer + /// fill on a utf8 column — has no bytes to broadcast. Falling back to the empty array + /// would render every unpatched row as a valid `""`, which is exactly the dropped-fill + /// bug this decode path just stopped having, so it must fail loud instead. + @Test + void utf8FillWithNonStringScalar_throws() { + // Given — a non-null i64 fill scalar on a utf8 column + MemorySegment[] segs = { + MemorySegment.ofArray(ProtoScalarValue.ofInt64Value(7L).encode()), + TestSegments.leInts(1), utf8Bytes("b"), TestSegments.leInts(0, 1)}; + + // When / Then + assertThatThrownBy(() -> decode(DType.UTF8, 1, 0, PType.U32, 3, segs, + primitiveNode(1), varBinNode(2, 3))) + .isInstanceOf(VortexException.class) + .hasMessageContaining("no string or bytes value"); + } + /// A null fill wraps the values in a [MaskedArray]; the adversarial reads above target /// the values array underneath it. private static VarBinArray assertVarBin(Array result) {