Skip to content

fix(reader): read vortex.bytebool in place instead of repacking it - #346

Merged
dfa1 merged 2 commits into
mainfrom
fix/bytebool-zero-copy
Aug 7, 2026
Merged

fix(reader): read vortex.bytebool in place instead of repacking it#346
dfa1 merged 2 commits into
mainfrom
fix/bytebool-zero-copy

Conversation

@dfa1

@dfa1 dfa1 commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Closes #339.

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. (Docs drift in the direction that flatters the code is the kind that survives longest — worth noting that the table was the thing that was right.)

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 want it rather than by every decode. segmentIfPresent stays empty (the interface default): 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, so handing it over would be misread rather than merely unhelpful.

ADR 0003 gap closed on the same line

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. It is now checked once at decode in O(1) — which is also what lets the carrier's accessor stay free of a per-row bound, keeping it uniform.

Worth a second opinion on: vortex.bool has the same shape of gap (MaterializedBoolArray.getBoolean indexes its buffer unchecked, and BoolEncodingDecoder passes the raw segment through), and I did not touch it — it's a wider change than this issue, and the packed layout makes "shorter than declared" a different calculation. Happy to file it.

Tests

  • LazyByteBoolArrayTest — new: any non-zero byte is true (the encoder writes 1, but the format doesn't promise it and a Rust-written file may not), a length shorter than the buffer hides trailing bytes, forEachBoolean order, segmentIfPresent empty, materialize packs LSB-first and agrees row-for-row with getBoolean over an irregular 20-row pattern.
  • ByteBoolEncodingDecoderTest — asserts the concrete carrier type (values alone would pass on the eager path too), plus the short-buffer rejection.

./mvnw verify green across all 17 modules.

🤖 Generated with Claude Code

@dfa1

dfa1 commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Pushed f153aa4: the vortex.bool gap I flagged above is now fixed in this PR rather than filed, since it's the same class of bug and this is where it surfaced.

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 out intact. Checked once at decode, in O(1).

The unchecked accessor stays unchecked deliberately: the other four construction sites (SparseEncodingDecoder, DictEncodingDecoder, DictLayoutDecoder, ChunkedArrayCombiner) all allocate the bitmap themselves at exactly (n + 7) >>> 3, so a per-row bound would be dead at every site but this one — and this one can answer it once.

The guard caught a real under-sized fixture on its first run, which is worth reading as evidence either way. RleEncodingEncoderTest#decode_nullableIndices_returnsMaskedArrayWithCorrectValidity hand-builds a nullable-indices node and gives it a 1-byte validity bitmap — but the indices child declares indices_len rows, which RleEncodingEncoder pads to a 1024 chunk boundary. Reading any row past 7 would have faulted; the test only ever read rows 0–3, so it passed. The fixture now sizes the bitmap for what the node declares.

Nothing else in the reactor trips it: full ./mvnw verify is green, including the Rust-written S3 fixtures and every writer round-trip. That's the check I wanted before believing the guard isn't over-strict — a broadcast/one-byte bitmap is not a legal vortex.bool shape (a constant validity is written as vortex.constant and decodes to LazyConstantBoolArray; AbstractMaterializedArray gives Bool elementCount == length, with no broadcast modulo), so there was no legitimate short-buffer case to preserve.

Tests: short bitmaps at one past each byte boundary (9 rows/1 byte, 17/2, 65/8, 8/0), the same guard reached through a validity child, and a longer-than-needed bitmap staying accepted.

dfa1 and others added 2 commits August 7, 2026 08:08
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@dfa1
dfa1 force-pushed the fix/bytebool-zero-copy branch from f153aa4 to 56e7719 Compare August 7, 2026 06:10
@dfa1
dfa1 merged commit 539aa14 into main Aug 7, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ByteBoolEncodingDecoder repacks eagerly; a LazyByteBoolArray would be zero-copy

1 participant