Skip to content

Commit 44982b7

Browse files
dfa1claude
andcommitted
fix(reader): skip the copy when vortex.patched has no patches
PatchedEncodingDecoder allocated n * elemBytes and broadcastCopy'd the inner child into it unconditionally, then applied patches only when there were any. With nPatches == 0 the result is byte-for-byte the inner child, which had just come from decodeChildSegment and is already arena-lifetime — so the decoder paid a full column copy to produce a duplicate. A zero-patch node is not exotic: a bitpacked column whose exception list happens to be empty for a chunk still round-trips through this encoding. The copy is not always avoidable. An inner child holding fewer than n elements is the ConstantEncoding fan-out that broadcastCopy exists for, so the alias is taken only when the child covers every row; the guard tests capacity, not just nPatches. The child is sliced to exactly n elements so the Materialized* accessors keep their `length == elementCount` fast path when the buffer runs long, and returned read-only, matching the sibling decoders. Tests assert the aliasing directly rather than just the values — the existing no-patch test passes either way — by mutating the source buffer after decode and observing the change, plus the two guard edges (inner longer than the row count, inner shorter than it). Closes #337 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent a4af152 commit 44982b7

3 files changed

Lines changed: 105 additions & 7 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1212
- A run-end-encoded Utf8/Binary column (`vortex.runend`) no longer expands every run into a fully materialized buffer on decode; rows now resolve through the runs lazily, removing an unbounded `sum(runLength * valueLength)` allocation that a crafted file could drive to `OutOfMemoryError`. ([#334](https://github.com/dfa1/vortex-java/issues/334))
1313
- 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))
1414
- 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))
15+
- 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))
1516

1617
## [0.13.1] — 2026-08-06
1718

reader/src/main/java/io/github/dfa1/vortex/reader/decode/PatchedEncodingDecoder.java

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,8 @@ public Array decode(DecodeContext ctx) {
6666
DType.U16, nPatches);
6767
MemorySegment patchValuesSeg = ctx.decodeChildSegment(3, ctx.dtype(), nPatches);
6868

69-
MemorySegment out = ctx.arena().allocate(n * elemBytes);
70-
SegmentBroadcast.broadcastCopy(innerSeg, out, n, elemBytes);
71-
72-
if (nPatches > 0) {
73-
applyPatches(out, n, nChunks, nLanes, offset, elemBytes,
74-
laneOffsetsSeg, patchIndicesSeg, patchValuesSeg);
75-
}
69+
MemorySegment out = patchedOutput(ctx, innerSeg, n, elemBytes, nPatches,
70+
nChunks, nLanes, offset, laneOffsetsSeg, patchIndicesSeg, patchValuesSeg);
7671

7772
return switch (ptype) {
7873
case I8, U8 -> new MaterializedByteArray(ctx.dtype(), n, out);
@@ -86,6 +81,47 @@ public Array decode(DecodeContext ctx) {
8681
};
8782
}
8883

84+
/// Produces the patched values buffer.
85+
///
86+
/// With no patches to apply, the result is byte-for-byte the inner child, so allocating
87+
/// `n * elemBytes` and copying into it produces a duplicate and nothing else — the inner
88+
/// segment is already arena-lifetime, having just come from `decodeChildSegment`. A
89+
/// zero-patch `vortex.patched` node is not exotic: a bitpacked column whose exception list
90+
/// happens to be empty for a chunk still round-trips through this encoding.
91+
///
92+
/// The copy is still required when the inner child holds fewer than `n` elements — the
93+
/// `ConstantEncoding` fan-out that [SegmentBroadcast#broadcastCopy] exists for — so the
94+
/// alias is taken only when the child covers every row. It is sliced to exactly `n`
95+
/// elements so the `Materialized*` accessors keep their `length == elementCount` fast path
96+
/// even when the child buffer runs long.
97+
///
98+
/// @param ctx decode context (allocation arena)
99+
/// @param innerSeg decoded inner child
100+
/// @param n logical row count
101+
/// @param elemBytes value element width
102+
/// @param nPatches number of patches declared by the metadata
103+
/// @param nChunks number of 1024-row chunks spanned
104+
/// @param nLanes lanes per chunk
105+
/// @param offset starting absolute position
106+
/// @param laneOffsetsSeg per-chunk lane offsets
107+
/// @param patchIndicesSeg per-patch in-chunk indices
108+
/// @param patchValuesSeg per-patch replacement values
109+
/// @return the values buffer, aliased to `innerSeg` when there is nothing to patch
110+
private static MemorySegment patchedOutput(DecodeContext ctx, MemorySegment innerSeg, long n, int elemBytes,
111+
long nPatches, long nChunks, long nLanes, long offset,
112+
MemorySegment laneOffsetsSeg, MemorySegment patchIndicesSeg, MemorySegment patchValuesSeg) {
113+
if (nPatches == 0 && SegmentBroadcast.capacity(innerSeg, elemBytes) >= n) {
114+
return innerSeg.asSlice(0, n * elemBytes).asReadOnly();
115+
}
116+
MemorySegment out = ctx.arena().allocate(n * elemBytes);
117+
SegmentBroadcast.broadcastCopy(innerSeg, out, n, elemBytes);
118+
if (nPatches > 0) {
119+
applyPatches(out, n, nChunks, nLanes, offset, elemBytes,
120+
laneOffsetsSeg, patchIndicesSeg, patchValuesSeg);
121+
}
122+
return out;
123+
}
124+
89125
private static void applyPatches(
90126
MemorySegment out, long n, long nChunks, long nLanes, long offset, int elemBytes,
91127
MemorySegment laneOffsets, MemorySegment patchIndices, MemorySegment patchValues

reader/src/test/java/io/github/dfa1/vortex/reader/decode/PatchedEncodingDecoderTest.java

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.github.dfa1.vortex.core.testing.TestSegments;
44
import io.github.dfa1.vortex.reader.ReadRegistry;
55

6+
import io.github.dfa1.vortex.core.io.VortexFormat;
67
import io.github.dfa1.vortex.core.model.DType;
78
import io.github.dfa1.vortex.reader.array.Array;
89
import io.github.dfa1.vortex.reader.array.IntArray;
@@ -77,6 +78,66 @@ void decode_noPatches_returnsInnerUnchanged() {
7778
}
7879
}
7980

81+
/// With nothing to patch, the output is byte-for-byte the inner child, so the decoder must
82+
/// alias it rather than allocate `n * elemBytes` and copy into a duplicate (#337). Proven by
83+
/// mutating the source after decode: a copy would not see the change.
84+
@Test
85+
void decode_noPatches_aliasesTheInnerChildInsteadOfCopying() {
86+
// Given — a mutable inner buffer, so a later write reveals whether it was copied
87+
byte[] backing = new byte[4 * Integer.BYTES];
88+
MemorySegment inner = MemorySegment.ofArray(backing);
89+
for (int i = 0; i < 4; i++) {
90+
inner.setAtIndex(VortexFormat.LE_INT, i, (i + 1) * 10);
91+
}
92+
93+
// When
94+
Array result = decodeNoPatches(inner, 4);
95+
inner.setAtIndex(VortexFormat.LE_INT, 2, 999);
96+
97+
// Then
98+
assertThat(((IntArray) result).getInt(2)).isEqualTo(999);
99+
}
100+
101+
/// An inner child longer than the row count is sliced to exactly `n` elements, so the
102+
/// `Materialized*` accessors keep their `length == elementCount` fast path instead of
103+
/// falling into the broadcast-modulo branch.
104+
@Test
105+
void decode_noPatches_innerLongerThanRowCount_isSlicedToRowCount() {
106+
// Given — 6 elements on the wire, 4 rows declared
107+
MemorySegment inner = TestSegments.leInts(10, 20, 30, 40, 50, 60);
108+
109+
// When
110+
Array result = decodeNoPatches(inner, 4);
111+
112+
// Then
113+
assertThat(result.length()).isEqualTo(4L);
114+
assertThat(result.segmentIfPresent()).hasValueSatisfying(
115+
seg -> assertThat(seg.byteSize()).isEqualTo(4L * Integer.BYTES));
116+
assertThat(((IntArray) result).getInt(3)).isEqualTo(40);
117+
}
118+
119+
/// The alias is only safe when the child covers every row. A `ConstantEncoding` child holds
120+
/// one element for any row count, so that case must still fan out through the copy.
121+
@Test
122+
void decode_noPatches_undersizedInner_stillBroadcasts() {
123+
// Given — a single inner element for 4 rows
124+
MemorySegment inner = TestSegments.leInts(7);
125+
126+
// When
127+
Array result = decodeNoPatches(inner, 4);
128+
129+
// Then
130+
IntArray ints = (IntArray) result;
131+
for (int i = 0; i < 4; i++) {
132+
assertThat(ints.getInt(i)).as("index %d", i).isEqualTo(7);
133+
}
134+
}
135+
136+
private static Array decodeNoPatches(MemorySegment inner, int n) {
137+
return decode(DType.I32, n, inner, TestSegments.leInts(0, 0),
138+
TestSegments.leShorts(), TestSegments.leInts(), 1);
139+
}
140+
80141
@Test
81142
void decode_singlePatch_overwrites() {
82143
// Given / When

0 commit comments

Comments
 (0)