From aafeeef08dd3944cb6ff4570db923031e38d04a5 Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Fri, 7 Aug 2026 07:30:12 +0200 Subject: [PATCH 1/2] fix(reader): read vortex.bytebool in place instead of repacking it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ByteBoolEncodingDecoder allocated an (n + 7) / 8 byte bitmap and ran a read-modify-write over every row to fill it, for the one boolean encoding whose source buffer is already directly indexable per row. The allocation is modest; the O(n) loop with a data-dependent branch and a read-modify-write per row is the more interesting cost, and neither was buying anything. docs/compatibility.md has claimed `vortex.bytebool` is "Zero-copy — mmap slice" the whole time. It now is. LazyByteBoolArray reads the mmapped bytes in place. Callers that need a real LSB-first bitmap still get one from BoolArray#materialize — the same packing loop, now paid by the callers that actually want it rather than by every decode. segmentIfPresent stays empty: the backing segment is byte-per-row, not the bit-packed layout a caller asking a bool array for its segment would read it as. Also closes an ADR 0003 gap on the line being rewritten. The buffer is untrusted and holds one byte per row, so a shorter one is malformed; the old loop faulted on whichever row ran off the end with a raw IndexOutOfBoundsException. Checked once at decode in O(1), which is also what lets the carrier's accessor stay free of a per-row bound. Closes #339 Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 3 + docs/compatibility.md | 2 +- .../reader/array/LazyByteBoolArray.java | 45 +++++ .../decode/ByteBoolEncodingDecoder.java | 26 +-- .../reader/array/LazyByteBoolArrayTest.java | 158 ++++++++++++++++++ .../decode/ByteBoolEncodingDecoderTest.java | 31 +++- 6 files changed, 248 insertions(+), 17 deletions(-) create mode 100644 reader/src/main/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArray.java create mode 100644 reader/src/test/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArrayTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index cd723ca5..60798814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- A `vortex.bytebool` column is now read in place from its mmapped buffer, as `docs/compatibility.md` already claimed: decode allocated an `n/8`-byte bitmap and ran a read-modify-write over every row to fill it, for the one boolean encoding whose buffer is already indexable per row. Callers that want a bitmap still get one from `materialize`. ([#339](https://github.com/dfa1/vortex-java/issues/339)) +- A `vortex.bytebool` buffer shorter than the declared row count now fails as `VortexException` instead of a raw `IndexOutOfBoundsException` on whichever row ran off the end. ([#339](https://github.com/dfa1/vortex-java/issues/339)) + - A malformed `fastlanes.delta` column no longer fails with a raw JDK exception: a row window running past the elements the chunks reconstruct threw `ArrayIndexOutOfBoundsException`, and an absurd or negative declared element count sized a heap array before anything checked it (`NegativeArraySizeException`, or `OutOfMemoryError`). All now fail as `VortexException`. ([#338](https://github.com/dfa1/vortex-java/issues/338)) - A `fastlanes.delta` column no longer routes its decode through four row-scaled heap `long[]` arrays, every value widened to 8 bytes whatever the column's width; values are reconstructed into a single arena segment at the ptype's real width, and only the chunks overlapping the requested rows are reconstructed at all. ([#338](https://github.com/dfa1/vortex-java/issues/338)) diff --git a/docs/compatibility.md b/docs/compatibility.md index e17b28bd..1b63a561 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -134,7 +134,7 @@ decoder falls into one of three shapes: | `vortex.primitive` | Zero-copy | Zero-copy | mmap slice | | `vortex.bool` | Zero-copy | Zero-copy | mmap slice (bit-packed) | | `vortex.null` | n/a | n/a | no per-row data | -| `vortex.bytebool` | Zero-copy | Zero-copy | mmap slice | +| `vortex.bytebool` | Zero-copy | Zero-copy | `LazyByteBoolArray` — mmap slice read byte-per-row; bitmap on `materialize` | | `vortex.zigzag` | Lazy | Lazy | `LazyZigZagXxxArray` (I8/I16/I32/I64); broadcast → `LazyConstantXxxArray`, ADR 0010 + 0015 | | `vortex.constant` | Lazy | Lazy | `LazyConstantXxxArray` (primitive + bool + decimal) + `VarBinConstantArray` (Utf8/Binary); per-row broadcast, no buffer, ADR 0015 | | `vortex.ext` | Zero-copy | Zero-copy | wraps storage | diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArray.java b/reader/src/main/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArray.java new file mode 100644 index 00000000..5e9c2f95 --- /dev/null +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArray.java @@ -0,0 +1,45 @@ +package io.github.dfa1.vortex.reader.array; + +import io.github.dfa1.vortex.core.model.DType; + +import java.lang.foreign.MemorySegment; +import java.lang.foreign.ValueLayout; +import java.util.Objects; + +/// Zero-copy [BoolArray] over a `vortex.bytebool` buffer: one byte per row, non-zero meaning +/// `true`. +/// +/// `vortex.bytebool` is the one boolean encoding whose source buffer is already directly +/// indexable per row, so decode has nothing to do — this reads the mmapped bytes in place +/// instead of allocating an `n/8`-byte bitmap and running a read-modify-write over every row +/// to fill it. +/// +/// [#materialize(java.lang.foreign.SegmentAllocator)] still hands out the LSB-first bitmap the +/// rest of the reader speaks, via [BoolArray]'s default — the same packing loop this decode +/// used to run eagerly, now paid only by callers that actually want a bitmap. +/// +/// [Array#segmentIfPresent()] is left empty (the interface default): the backing segment is a +/// byte-per-row buffer, not the bit-packed layout a caller asking for a bool array's segment +/// expects, so handing it over would be misread. +/// +/// @param dtype logical [DType.Bool] type +/// @param length number of logical rows +/// @param bytes one byte per row; non-zero is `true`. Must hold at least `length` bytes — +/// [io.github.dfa1.vortex.reader.decode.ByteBoolEncodingDecoder] checks that +/// once, so the accessors here do not re-check it per row +public record LazyByteBoolArray(DType dtype, long length, MemorySegment bytes) implements BoolArray { + + @Override + public boolean getBoolean(long i) { + Objects.checkIndex(i, length); + return bytes.get(ValueLayout.JAVA_BYTE, i) != 0; + } + + @Override + public void forEachBoolean(BooleanConsumer c) { + long n = length; + for (long i = 0; i < n; i++) { + c.accept(bytes.get(ValueLayout.JAVA_BYTE, i) != 0); + } + } +} diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoder.java b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoder.java index 1d95f53f..5427e93f 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoder.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoder.java @@ -1,14 +1,13 @@ package io.github.dfa1.vortex.reader.decode; +import io.github.dfa1.vortex.core.error.VortexException; import io.github.dfa1.vortex.core.model.EncodingId; import io.github.dfa1.vortex.reader.array.Array; -import io.github.dfa1.vortex.reader.array.MaterializedBoolArray; +import io.github.dfa1.vortex.reader.array.LazyByteBoolArray; import java.lang.foreign.MemorySegment; -import java.lang.foreign.ValueLayout; -/// Read-only decoder for `vortex.bytebool` — packs the input byte buffer into the -/// bit-packed [MaterializedBoolArray] layout used by `vortex.bool`. +/// Read-only decoder for `vortex.bytebool` — one byte per boolean, read in place. public final class ByteBoolEncodingDecoder implements EncodingDecoder { @Override @@ -20,15 +19,16 @@ public EncodingId encodingId() { public Array decode(DecodeContext ctx) { long n = ctx.rowCount(); MemorySegment bytes = ctx.buffer(0); - long packedBytes = (n + 7) >>> 3; - MemorySegment packed = ctx.arena().allocate(packedBytes > 0 ? packedBytes : 1); - for (long i = 0; i < n; i++) { - if (bytes.get(ValueLayout.JAVA_BYTE, i) != 0) { - long byteIdx = i >>> 3; - byte cur = packed.get(ValueLayout.JAVA_BYTE, byteIdx); - packed.set(ValueLayout.JAVA_BYTE, byteIdx, (byte) ((cur & 0xff) | (1 << (i & 7)))); - } + // The buffer comes straight from the file and holds one byte per row, so a shorter one + // is malformed. Checked once here, in O(1), rather than per row: it keeps + // LazyByteBoolArray's accessor uniform, and a crafted file fails as a VortexException + // instead of a raw IndexOutOfBoundsException on whichever row runs off the end + // (ADR 0003) — which is what the eager packing loop this replaces did. + if (bytes.byteSize() < n) { + throw new VortexException(EncodingId.VORTEX_BYTEBOOL, + "bytebool buffer of " + bytes.byteSize() + " byte(s) is shorter than the " + + n + " declared row(s)"); } - return new MaterializedBoolArray(ctx.dtype(), n, packed); + return new LazyByteBoolArray(ctx.dtype(), n, bytes); } } diff --git a/reader/src/test/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArrayTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArrayTest.java new file mode 100644 index 00000000..1d6267b6 --- /dev/null +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/array/LazyByteBoolArrayTest.java @@ -0,0 +1,158 @@ +package io.github.dfa1.vortex.reader.array; + +import io.github.dfa1.vortex.core.model.DType; +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.lang.foreign.ValueLayout; +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 LazyByteBoolArrayTest { + + private static final DType BOOL = DType.BOOL; + + @Nested + class Accessors { + + /// Any non-zero byte is `true`, not just 1 — the encoder writes 1, but the format does + /// not promise it and a Rust-written file may carry other values. + @ParameterizedTest + @CsvSource({"0,false", "1,true", "2,true", "42,true", "-1,true", "-128,true"}) + void getBoolean_treatsAnyNonZeroByteAsTrue(byte value, boolean expected) { + // Given + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 1, bytes(value)); + + // When + boolean result = sut.getBoolean(0); + + // Then + assertThat(result).isEqualTo(expected); + } + + @Test + void getBoolean_resolvesEachRowIndependently() { + // Given + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 4, bytes((byte) 0, (byte) 1, (byte) 0, (byte) 1)); + + // When / Then + assertThat(sut.getBoolean(0)).isFalse(); + assertThat(sut.getBoolean(1)).isTrue(); + assertThat(sut.getBoolean(2)).isFalse(); + assertThat(sut.getBoolean(3)).isTrue(); + } + + @Test + void getBoolean_outOfBoundsIndex_throws() { + // Given + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 2, bytes((byte) 1, (byte) 0)); + + // When / Then + assertThatThrownBy(() -> sut.getBoolean(2)).isInstanceOf(IndexOutOfBoundsException.class); + } + + /// A shorter `length` than the buffer holds is how a sliced or trimmed column arrives; + /// the trailing bytes must stay invisible. + @Test + void getBoolean_lengthShorterThanBuffer_hidesTrailingBytes() { + // Given — 4 bytes of data but only 2 rows claimed + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 2, bytes((byte) 1, (byte) 0, (byte) 1, (byte) 1)); + + // When / Then + assertThat(sut.getBoolean(1)).isFalse(); + assertThatThrownBy(() -> sut.getBoolean(2)).isInstanceOf(IndexOutOfBoundsException.class); + } + + @Test + void forEachBoolean_visitsEveryRowInOrder() { + // Given + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 3, bytes((byte) 1, (byte) 0, (byte) 1)); + List seen = new ArrayList<>(); + + // When + sut.forEachBoolean(seen::add); + + // Then + assertThat(seen).containsExactly(true, false, true); + } + } + + @Nested + class Representation { + + /// The byte-per-row buffer is not the LSB-first bitmap a caller asking a bool array for + /// its segment expects, so it must not be handed over as one. + @Test + void segmentIfPresent_isEmpty() { + // Given + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 2, bytes((byte) 1, (byte) 0)); + + // When / Then + assertThat(sut.segmentIfPresent()).isEmpty(); + } + + /// The bit-packing the decoder used to do eagerly, now on demand: bits are LSB-first, + /// so rows 0 and 9 land in bit 0 of bytes 0 and 1. + @Test + void materialize_packsLsbFirstBitmap() { + // Given — 10 rows, true at 0 and 9 + byte[] raw = new byte[10]; + raw[0] = 1; + raw[9] = 1; + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 10, bytes(raw)); + + // When + MemorySegment result = sut.materialize(Arena.ofAuto()); + + // Then + assertThat(result.byteSize()).isEqualTo(2); + assertThat(result.get(ValueLayout.JAVA_BYTE, 0)).isEqualTo((byte) 0b0000_0001); + assertThat(result.get(ValueLayout.JAVA_BYTE, 1)).isEqualTo((byte) 0b0000_0010); + } + + /// Round-trip through the bitmap must agree with reading the bytes directly — the two + /// are what a consumer picks between, so they cannot disagree. + @Test + void materialize_agreesWithGetBoolean() { + // Given — 20 rows with an irregular pattern, so a byte-boundary slip is visible + byte[] raw = new byte[20]; + for (int i = 0; i < raw.length; i++) { + raw[i] = (byte) (i % 3 == 0 ? 1 : 0); + } + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, raw.length, bytes(raw)); + + // When + MemorySegment result = sut.materialize(Arena.ofAuto()); + + // Then + BoolArray packed = new MaterializedBoolArray(BOOL, raw.length, result); + for (long i = 0; i < raw.length; i++) { + assertThat(packed.getBoolean(i)).as("row %d", i).isEqualTo(sut.getBoolean(i)); + } + } + + @Test + void limited_capsTheRowCount() { + // Given + LazyByteBoolArray sut = new LazyByteBoolArray(BOOL, 4, bytes((byte) 1, (byte) 0, (byte) 1, (byte) 1)); + + // When + Array result = sut.limited(2); + + // Then + assertThat(result.length()).isEqualTo(2); + assertThat(((BoolArray) result).getBoolean(1)).isFalse(); + } + } + + private static MemorySegment bytes(byte... values) { + return MemorySegment.ofArray(values); + } +} diff --git a/reader/src/test/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoderTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoderTest.java index 96f421f4..2b5d9764 100644 --- a/reader/src/test/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoderTest.java +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/decode/ByteBoolEncodingDecoderTest.java @@ -2,10 +2,13 @@ import io.github.dfa1.vortex.reader.ReadRegistry; +import io.github.dfa1.vortex.core.error.VortexException; import io.github.dfa1.vortex.reader.array.BoolArray; +import io.github.dfa1.vortex.reader.array.LazyByteBoolArray; import io.github.dfa1.vortex.core.testing.DTypes; import io.github.dfa1.vortex.core.model.EncodingId; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -15,6 +18,7 @@ import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; class ByteBoolEncodingDecoderTest { @@ -37,7 +41,7 @@ private static DecodeContext buildCtx(byte[] byteValues) { @ParameterizedTest(name = "{0}") @MethodSource("cases") - void decode_byteBool_packsToBitArray(String name, byte[] input, boolean[] expected) { + void decode_byteBool_readsBytesInPlace(String name, byte[] input, boolean[] expected) { // Given DecodeContext ctx = buildCtx(input); var sut = new ByteBoolEncodingDecoder(); @@ -45,12 +49,33 @@ void decode_byteBool_packsToBitArray(String name, byte[] input, boolean[] expect // When var result = sut.decode(ctx); - // Then - assertThat(result).isInstanceOf(BoolArray.class); + // Then — the concrete type is asserted because the values alone would also pass on the + // eager bit-packing path this replaced + assertThat(result).isInstanceOf(LazyByteBoolArray.class); assertThat(result.length()).isEqualTo(expected.length); BoolArray boolArr = (BoolArray) result; for (int i = 0; i < expected.length; i++) { assertThat(boolArr.getBoolean(i)).as("index %d", i).isEqualTo(expected[i]); } } + + /// The buffer is untrusted and holds one byte per row, so a shorter one is malformed. The + /// eager packing loop faulted on whichever row ran off the end, as a raw + /// `IndexOutOfBoundsException`; the reader must fail as a [VortexException] (ADR 0003), and + /// checking once at decode also keeps the carrier's accessor free of a per-row bound. + @Test + void decode_bufferShorterThanRowCount_throws() { + // Given — 3 bytes of data but 8 declared rows + MemorySegment buf = MemorySegment.ofArray(new byte[]{1, 0, 1}); + ArrayNode node = new ArrayNode(EncodingId.VORTEX_BYTEBOOL, null, new ArrayNode[0], new int[]{0}); + ReadRegistry registry = TestRegistry.ofDecoders(new ByteBoolEncodingDecoder()); + DecodeContext ctx = new DecodeContext(node, DTypes.BOOL, 8, new MemorySegment[]{buf}, registry, + Arena.ofAuto()); + var sut = new ByteBoolEncodingDecoder(); + + // When / Then + assertThatThrownBy(() -> sut.decode(ctx)) + .isInstanceOf(VortexException.class) + .hasMessageContaining("shorter than the 8 declared row(s)"); + } } From 56e77198a6a722a2192643fa44b0bb07c7f69243 Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Fri, 7 Aug 2026 07:48:18 +0200 Subject: [PATCH 2/2] fix(reader): reject a vortex.bool bitmap shorter than its row count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gap #339 exposed one encoding over. MaterializedBoolArray#getBoolean indexes its buffer with no bounds check, and BoolEncodingDecoder handed it a file buffer straight through, so a truncated bitmap faulted with a raw IndexOutOfBoundsException on whichever row ran off the end — and `materialize` handed the short buffer to the caller intact. Checked once at decode, in O(1). The unchecked accessor stays unchecked on purpose: the other four construction sites all allocate the bitmap themselves at exactly (n + 7) / 8, so a per-row bound there would be dead at every site but this one, and this one can answer it once. The guard found a real under-sized fixture on its first run. RleEncodingEncoderTest's hand-built nullable-indices node supplied a 1-byte validity bitmap for an indices child that declares `indices_len` rows — which the encoder pads to a 1024 chunk boundary. Reading any row past 7 would have faulted; the test only ever read rows 0 through 3. The fixture now sizes the bitmap for what the node declares. Nothing else in the reactor, writer output or Rust fixture alike, supplied a short one. Companion to the #339 bytebool fix in this PR; same class of bug, and the bytebool PR is where it was spotted. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + .../reader/array/MaterializedBoolArray.java | 6 ++ .../reader/decode/BoolEncodingDecoder.java | 17 ++++- .../decode/BoolEncodingDecoderTest.java | 70 +++++++++++++++++++ .../writer/encode/RleEncodingEncoderTest.java | 8 ++- 5 files changed, 99 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60798814..6d0e2b61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A `vortex.bytebool` column is now read in place from its mmapped buffer, as `docs/compatibility.md` already claimed: decode allocated an `n/8`-byte bitmap and ran a read-modify-write over every row to fill it, for the one boolean encoding whose buffer is already indexable per row. Callers that want a bitmap still get one from `materialize`. ([#339](https://github.com/dfa1/vortex-java/issues/339)) - A `vortex.bytebool` buffer shorter than the declared row count now fails as `VortexException` instead of a raw `IndexOutOfBoundsException` on whichever row ran off the end. ([#339](https://github.com/dfa1/vortex-java/issues/339)) +- Same for a `vortex.bool` bitmap holding fewer than the `(rows + 7) / 8` bytes it needs, reached either as a column or as another column's validity child. ([#339](https://github.com/dfa1/vortex-java/issues/339)) - A malformed `fastlanes.delta` column no longer fails with a raw JDK exception: a row window running past the elements the chunks reconstruct threw `ArrayIndexOutOfBoundsException`, and an absurd or negative declared element count sized a heap array before anything checked it (`NegativeArraySizeException`, or `OutOfMemoryError`). All now fail as `VortexException`. ([#338](https://github.com/dfa1/vortex-java/issues/338)) - A `fastlanes.delta` column no longer routes its decode through four row-scaled heap `long[]` arrays, every value widened to 8 bytes whatever the column's width; values are reconstructed into a single arena segment at the ptype's real width, and only the chunks overlapping the requested rows are reconstructed at all. ([#338](https://github.com/dfa1/vortex-java/issues/338)) diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/array/MaterializedBoolArray.java b/reader/src/main/java/io/github/dfa1/vortex/reader/array/MaterializedBoolArray.java index 0f1cb791..9b8691ca 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/array/MaterializedBoolArray.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/array/MaterializedBoolArray.java @@ -8,6 +8,12 @@ /// Buffer-backed [BoolArray] — the fallback used when an encoding decoder /// either materializes the output eagerly or has no lazy variant of its own. +/// +/// [#getBoolean(long)] indexes `buffer` without a bounds check, so the buffer must hold at +/// least `(length + 7) / 8` bytes. Every decoder that builds the bitmap itself allocates +/// exactly that; the one that passes a file buffer through +/// (`io.github.dfa1.vortex.reader.decode.BoolEncodingDecoder`) checks the size once before +/// constructing this, rather than paying a bound per row. public final class MaterializedBoolArray extends AbstractMaterializedArray implements BoolArray { /// Constructs a `MaterializedBoolArray` backed by the given bit-packed buffer. diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoder.java b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoder.java index 8a4f1c2b..6eed49a7 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoder.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoder.java @@ -8,6 +8,8 @@ import io.github.dfa1.vortex.reader.array.MaskedArray; import io.github.dfa1.vortex.reader.array.MaterializedBoolArray; +import java.lang.foreign.MemorySegment; + /// Read-only decoder for `vortex.bool` (bit-packed boolean arrays, LSB first). /// /// When the encoding node has one child, that child is the validity bitmask: @@ -23,7 +25,20 @@ public EncodingId encodingId() { @Override public Array decode(DecodeContext ctx) { long n = ctx.rowCount(); - Array values = new MaterializedBoolArray(ctx.dtype(), n, ctx.buffer(0)); + MemorySegment bits = ctx.buffer(0); + // The bitmap comes straight from the file and needs one byte per 8 rows. A shorter one + // is malformed, and [MaterializedBoolArray#getBoolean] indexes its buffer unchecked — + // deliberately, since every other construction site allocates the bitmap itself at + // exactly this size — so without this the read of whichever row runs off the end is a + // raw IndexOutOfBoundsException (ADR 0003). O(1), and it also covers `materialize`, + // which hands the same short buffer straight to the caller. + long needed = (n + 7) >>> 3; + if (bits.byteSize() < needed) { + throw new VortexException(EncodingId.VORTEX_BOOL, + "bool bitmap of " + bits.byteSize() + " byte(s) is shorter than the " + + needed + " byte(s) needed for " + n + " row(s)"); + } + Array values = new MaterializedBoolArray(ctx.dtype(), n, bits); if (ctx.node().children().length == 1) { Array va = ctx.decodeChild(0, DType.BOOL, n); if (!(va instanceof BoolArray validity)) { diff --git a/reader/src/test/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoderTest.java b/reader/src/test/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoderTest.java index 48e7cf29..15ccc3cb 100644 --- a/reader/src/test/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoderTest.java +++ b/reader/src/test/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoderTest.java @@ -1,5 +1,6 @@ package io.github.dfa1.vortex.reader.decode; +import io.github.dfa1.vortex.core.error.VortexException; import io.github.dfa1.vortex.core.model.EncodingId; import io.github.dfa1.vortex.core.testing.DTypes; import io.github.dfa1.vortex.reader.ReadRegistry; @@ -9,6 +10,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.MethodSource; import java.lang.foreign.Arena; @@ -16,6 +18,7 @@ import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; class BoolEncodingDecoderTest { @@ -105,6 +108,73 @@ void decode_nullable_returnsMaskedArray_withNullsHidingUnderlying() { assertThat(inner.getBoolean(2)).isFalse(); } + /// The bitmap is untrusted and needs one byte per 8 rows. `MaterializedBoolArray` indexes + /// its buffer without a per-row bound — deliberately, since every other decoder that builds + /// a bitmap allocates it at exactly the right size — so a short one used to fault as a raw + /// `IndexOutOfBoundsException` on whichever row ran off the end, or hand the truncated + /// buffer straight out of `materialize`. It must be a [VortexException] (ADR 0003). + /// + /// The row counts are one past each byte boundary, where a bitmap is at its most + /// deceptive: 9 rows need 2 bytes, not the 1 that covers the first 8. + @ParameterizedTest + @CsvSource({"9, 1", "17, 2", "65, 8", "8, 0"}) + void decode_bitmapShorterThanRowCount_throws(int rows, int suppliedBytes) { + // Given + MemorySegment bits = MemorySegment.ofArray(new byte[suppliedBytes]); + ArrayNode node = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0], new int[]{0}); + ReadRegistry registry = TestRegistry.ofDecoders(new BoolEncodingDecoder()); + DecodeContext ctx = new DecodeContext(node, DTypes.BOOL, rows, new MemorySegment[]{bits}, + registry, Arena.ofAuto()); + var sut = new BoolEncodingDecoder(); + + // When / Then + assertThatThrownBy(() -> sut.decode(ctx)) + .isInstanceOf(VortexException.class) + .hasMessageContaining("bool bitmap"); + } + + /// The same guard reached through the validity child, which decodes as its own + /// `vortex.bool` array: a values bitmap long enough for the row count paired with a + /// truncated validity bitmap must fail just as loudly. + @Test + void decode_nullable_validityBitmapTooShort_throws() { + // Given — 9 rows of values (2 bytes) but a 1-byte validity bitmap + MemorySegment bits = MemorySegment.ofArray(new byte[2]); + MemorySegment validBits = MemorySegment.ofArray(new byte[1]); + ArrayNode validityNode = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0], new int[]{1}); + ArrayNode node = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[]{validityNode}, new int[]{0}); + ReadRegistry registry = TestRegistry.ofDecoders(new BoolEncodingDecoder()); + DecodeContext ctx = new DecodeContext(node, DTypes.BOOL_N, 9, + new MemorySegment[]{bits, validBits}, registry, Arena.ofAuto()); + var sut = new BoolEncodingDecoder(); + + // When / Then + assertThatThrownBy(() -> sut.decode(ctx)) + .isInstanceOf(VortexException.class) + .hasMessageContaining("bool bitmap"); + } + + /// A bitmap longer than the row count needs is legal — trailing padding, or a buffer shared + /// with a longer array — and must not be rejected by the size guard. + @Test + void decode_bitmapLongerThanNeeded_isAccepted() { + // Given — 3 rows (1 byte needed) over an 8-byte buffer with bit 0 set + MemorySegment bits = MemorySegment.ofArray(new byte[]{1, 0, 0, 0, 0, 0, 0, 0}); + ArrayNode node = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0], new int[]{0}); + ReadRegistry registry = TestRegistry.ofDecoders(new BoolEncodingDecoder()); + DecodeContext ctx = new DecodeContext(node, DTypes.BOOL, 3, new MemorySegment[]{bits}, + registry, Arena.ofAuto()); + var sut = new BoolEncodingDecoder(); + + // When + var result = sut.decode(ctx); + + // Then + assertThat(result.length()).isEqualTo(3); + assertThat(((BoolArray) result).getBoolean(0)).isTrue(); + assertThat(((BoolArray) result).getBoolean(1)).isFalse(); + } + @Test void decode_nullable_allNulls_allRowsInvalid() { // Given — every validity bit is false; values buffer content does not matter diff --git a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoderTest.java b/writer/src/test/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoderTest.java index d0f8c051..7b3fe37a 100644 --- a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoderTest.java +++ b/writer/src/test/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoderTest.java @@ -315,8 +315,12 @@ void decode_nullableIndices_returnsMaskedArrayWithCorrectValidity() { EncodeResult encoded = ENCODER.encode(dtype, data, EncodeTestHelper.testCtx()); List originalBufs = new ArrayList<>(encoded.buffers()); - MemorySegment validityBuf = MemorySegment.ofArray(new byte[]{0x05}); - originalBufs.add(validityBuf); + // The indices child declares `indices_len` rows, which the encoder pads to a 1024 + // chunk boundary, so its validity bitmap has to cover all 1024 — not just the four + // rows this fixture cares about. Bits 0 and 2 set: rows 0 and 2 valid, 1 and 3 null. + byte[] validityBits = new byte[(1024 + 7) / 8]; + validityBits[0] = 0x05; + originalBufs.add(MemorySegment.ofArray(validityBits)); MemorySegment[] segments = originalBufs.toArray(new MemorySegment[0]); ArrayNode origRoot = toArrayNode(encoded.rootNode());