diff --git a/CHANGELOG.md b/CHANGELOG.md index ddad979d..a4bc84c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +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`) 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 ddd70c4b..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 | @@ -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) + `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/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/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 741f058f..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 @@ -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; @@ -26,11 +25,12 @@ 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.VarBinOffsetArray; +import io.github.dfa1.vortex.reader.array.VarBinConstantArray; +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 { @@ -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 @@ -96,7 +105,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) { @@ -258,7 +267,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. @@ -266,11 +276,12 @@ 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, fillValid); - 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, 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); } @@ -283,81 +294,36 @@ 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. 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 + /// @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]; } private static long scalarToLong(ProtoScalarValue scalar) { 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/array/VarBinSparseArrayTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java new file mode 100644 index 00000000..370b992c --- /dev/null +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/array/VarBinSparseArrayTest.java @@ -0,0 +1,275 @@ +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[] result = sut.getBytes(0); + result[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. 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 - offset, offset); + 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 8b5a0e8d..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 @@ -16,12 +16,16 @@ 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 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; @@ -190,6 +194,123 @@ 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). + @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 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)).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 + /// 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). @@ -316,12 +437,21 @@ 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. + /// 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, 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_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 @@ -329,19 +459,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), @@ -349,17 +480,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(), @@ -368,11 +504,72 @@ void varBinValueOffsetsBeyondValueBuffer_throws() { TestSegments.leInts(0, 1, 1 << 30) }; + // 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("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(nullableUtf8(), 2, 0, PType.U32, 5, segs, + assertThatThrownBy(() -> decode(DType.UTF8, 1, -1, PType.U32, 6, segs, primitiveNode(1), varBinNode(2, 3))) .isInstanceOf(VortexException.class) - .hasMessageContaining("patch bytes"); + .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) { + Array inner = result instanceof MaskedArray m ? m.inner() : result; + assertThat(inner).isInstanceOf(VarBinArray.class); + return (VarBinArray) inner; } }