fix(reader): decode fastlanes.delta into the arena, not four heap long[] - #345
Merged
Conversation
DeltaEncodingDecoder routed the whole column through four row-scaled heap long[] arrays before writing one arena segment: bases and deltas copied out of their segments, a `decoded` array for the reconstruction, and a `result` array whose only job was to drop `offset` leading elements — a second full traversal for a slice. Every value was widened to 8 bytes whatever the column's width, so an I8 delta column allocated 8x its natural size on the GC heap, three times over. That is CLAUDE.md's allocation rule violated four times at row scale. Values are now reconstructed into a single `ctx.arena()` segment at the ptype's real width. The per-chunk scratch stays on the heap, which is what it is for: fixed-size, cache-resident, reused across chunks. readLongs carried both hot-loop anti-patterns at once — an `i % cap` per element and a per-element `switch (ptype)`. Both are hoisted: readElements branch-splits on whether the segment physically holds the range, so the fast path is a uniform modulo-free loop per ptype and the wrap-around stays on the cold path, where only a vortex.constant child reaches it. The write side gets the same treatment, replacing PrimitiveArrays.fromLongs' per-element PTypeIO.set switch. Chunks are independent — each carries its own lane bases — so only those overlapping the requested row window are reconstructed at all. Reading the tail of a long column no longer walks every chunk before it. Fixes three ADR 0003 violations on the way: a row window past the elements the chunks reconstruct reached System.arraycopy as a raw ArrayIndexOutOfBoundsException; a negative deltas_len sized a heap array (NegativeArraySizeException); and an absurd one was truncated by an (int) cast into either a negative size or an OutOfMemoryError. The window is now validated before any child decode, so bogus metadata never drives an allocation, and a legal window over an absurd declared length simply decodes. Coverage: the decoder had three unit tests, all I64, all single-element children, none past one chunk. Adds a Java-write/Java-read round-trip over all eight integer widths across three chunks with full-width random bit patterns (the high bit is where sign- and zero-extension diverge), a test that a non-zero offset window is exactly the slice of the full decode (the writer always emits offset 0, so that path was unreachable from a file), and the malformed-metadata cases above. Closes #338 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review follow-up on #338. `rowCount > deltasLen - offset` was the only thing standing between a negative `deltas_len` and the chunk loop, and the subtraction wraps: `Long.MIN_VALUE - 1` is positive, so the window check passed, the chunk range came out empty, and decode handed back a zero-filled array of the requested length. A malformed file answered instead of rejected — no raw exception, so ADR 0003 held, but the file is still garbage and should say so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Self-reviewed the diff against the chunk-window arithmetic, the guard, the broadcast path, and per-ptype value equivalence (the review agent hit a session limit mid-run). Four of the five check out:
One real hole, fixed in ea7b933: the guard leaned on
|
This was referenced Aug 7, 2026
dfa1
added a commit
that referenced
this pull request
Aug 7, 2026
…ndow Takes the better half of #343, which fixed #338 independently and in parallel — I merged #345 for the same issue without checking for an open PR first, so this reconciles the two rather than discarding one. From #343: - scatterChunk writes each untransposed value straight to its output index, dropping the chunk-sized `untransposed` buffer #345 staged it in and the separate pass that sliced it. The leading chunk of an offset-sliced array maps to a negative index and the trailing chunk runs past the row count; one `Long.compareUnsigned` covers both, since a negative index reads as a huge unsigned value. The stores are a permutation scatter and never vectorize regardless, so the compare costs nothing the untranspose was not already paying. - The returned segment is read-only. - Element-indexed `getAtIndex` instead of hand-computed byte offsets. - Reader-module tests that build the delta wire form directly, mirroring DeltaEncodingEncoder's layout. These cover offset slicing where it belongs — the writer is not on the reader's test classpath and never emits a non-zero offset, so #345 had to reach into the integration module to cover the same shape. Kept from #345: - Only chunks overlapping the row window are reconstructed. #343 walked every chunk and discarded the out-of-window stores per element, so a one-chunk slice of a thousand-chunk column did a thousand chunks of work. - The metadata range guard. Without it a `deltas_len` of Long.MAX_VALUE drives the chunk loop ~9e15 times — a hang, which is worse than the OutOfMemoryError it replaced. Closes #343. Co-Authored-By: Davide Angelocola <davide.angelocola@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #338.
The allocation
DeltaEncodingDecoderrouted the whole column through four row-scaled heaplong[]arrays before writing one arena segment:basesAll,deltasAll, adecodedarray for the reconstruction, and aresultarray whose only job was to dropoffsetleading elements — a second full traversal to express a slice. Every value was widened to 8 bytes whatever the column's width, so an I8 delta column allocated 8× its natural size on the GC heap, three times over, plus the final off-heap segment.Values now land in a single
ctx.arena()segment at the ptype's real width. The per-chunk scratch stays on the heap, which is what it's for: fixed-size, cache-resident, reused across chunks.The hot loops
readLongscarried both anti-patterns from CLAUDE.md at once — ani % capper element and a per-elementswitch (ptype):readElementsbranch-splits on whether the segment physically holds the range: the fast path is a uniform, modulo-free loop per ptype; the wrap-around arithmetic stays on the cold path, where only avortex.constantchild ever reaches it. The write side gets the same treatment, which also dropsPrimitiveArrays.fromLongs' per-elementPTypeIO.setswitch.Only the chunks you asked for
Chunks are independent — each carries its own lane bases — so only those overlapping the requested row window are reconstructed. Reading the tail of a long column no longer walks every chunk before it.
Three ADR 0003 violations fixed on the way
deltas_lenandoffsetare untrusted metadata, and nothing checked them:ArrayIndexOutOfBoundsExceptionfromSystem.arraycopyVortexExceptiondeltas_lenNegativeArraySizeExceptionfromnew long[(int) deltasLen]VortexExceptiondeltas_len=Long.MAX_VALUE, 4 rows(int)cast → negative size orOutOfMemoryErrorThe window is validated before any child decode, so bogus metadata never drives an allocation.
Coverage
The decoder had three unit tests: all I64, all single-element (constant) children, none past one chunk. Added:
JavaRoundTripIntegrationTest#delta_javaWriteJavaRead— write→read over all eight integer widths, 2500 rows (three chunks), with full-width random bit patterns rather than a monotonic ramp, since the high bit is exactly where a sign-extending read and a zero-extending one diverge. Asserts the chosen encoding too, so a writer that stopped picking delta fails here instead of leaving the decoder untested.#delta_offsetWindowIsTheSliceOfTheFullDecode— the Java writer always emitsoffset = 0, so a non-zero offset only arrives on a Rust-written sliced array and was unreachable from a file. This drives the decoder over encoder-produced children and asserts the window is exactly the slice of the full decode, across a window that opens inside chunk 0 and closes inside chunk 2.DeltaEncodingDecoderTest— the malformed-metadata cases above.Not done
No benchmark. The issue suggested a
JavaVsJniReadBenchmark-style before/after; there is no delta benchmark today (delta is unstable-edition and not in the default write path), and I'd rather claim the allocation change, which is structural and visible in the diff, than a speedup I haven't measured. Worth its own pass if delta enters the default cascade../mvnw verifygreen across all 17 modules.🤖 Generated with Claude Code