From 2121943177b873fd9e4051f709bf23091a902101 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sat, 5 Sep 2026 11:28:41 +0800 Subject: [PATCH] [common] Do not zero the HeapBytesVector buffer on reset reset() ran Arrays.fill(buffer, 0) directly under the comment saying it does not reset the buffer. Besides costing O(buffer size) per batch, it corrupts values: skipBinary in VectorizedDeltaByteArrayReader alternates two vectors and leaves previous pointing into the last one written, so after an odd number of skipped values the next skip call resets that vector and copies a zeroed prefix out of it. Reads are bounded by the start/length arrays, which reset() still clears, so dropping the fill is safe. --- .../data/columnar/heap/HeapBytesVector.java | 2 -- .../heap/HeapBytesVectorReserveBytesTest.java | 16 ++++++++++++++++ .../reader/VectorizedDeltaByteArrayReader.java | 10 +++++----- .../reader/DeltaByteArrayEncodingTest.java | 17 +++++++++++++++++ 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/paimon-common/src/main/java/org/apache/paimon/data/columnar/heap/HeapBytesVector.java b/paimon-common/src/main/java/org/apache/paimon/data/columnar/heap/HeapBytesVector.java index 3c57c9bd8003..cbea2f364088 100644 --- a/paimon-common/src/main/java/org/apache/paimon/data/columnar/heap/HeapBytesVector.java +++ b/paimon-common/src/main/java/org/apache/paimon/data/columnar/heap/HeapBytesVector.java @@ -74,8 +74,6 @@ public void reset() { } // We don't reset buffer to avoid unnecessary copy. - Arrays.fill(buffer, (byte) 0); - this.bytesAppended = 0; } diff --git a/paimon-common/src/test/java/org/apache/paimon/data/columnar/heap/HeapBytesVectorReserveBytesTest.java b/paimon-common/src/test/java/org/apache/paimon/data/columnar/heap/HeapBytesVectorReserveBytesTest.java index b02c745968f5..ac39cded4d7d 100644 --- a/paimon-common/src/test/java/org/apache/paimon/data/columnar/heap/HeapBytesVectorReserveBytesTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/data/columnar/heap/HeapBytesVectorReserveBytesTest.java @@ -179,4 +179,20 @@ void testResetClearsBytesAppended() { assertThat(bytes.len).isEqualTo(2); assertThat(vector.buffer[0]).isEqualTo((byte) 10); } + + @Test + void testResetDoesNotWipeBuffer() { + HeapBytesVector vector = new HeapBytesVector(4); + byte[] data = new byte[] {1, 2, 3}; + vector.putByteArray(0, data, 0, data.length); + + vector.reset(); + + // reset() deliberately leaves the data buffer untouched: wiping it costs + // O(buffer size) per batch in the vectorized reader hot path, and reads are + // always bounded by the start/length offsets, which reset() does clear + assertThat(vector.buffer[0]).isEqualTo((byte) 1); + assertThat(vector.start[0]).isZero(); + assertThat(vector.length[0]).isZero(); + } } diff --git a/paimon-format/src/main/java/org/apache/paimon/format/parquet/reader/VectorizedDeltaByteArrayReader.java b/paimon-format/src/main/java/org/apache/paimon/format/parquet/reader/VectorizedDeltaByteArrayReader.java index 05ca1bacbbc8..41a7cdedf066 100644 --- a/paimon-format/src/main/java/org/apache/paimon/format/parquet/reader/VectorizedDeltaByteArrayReader.java +++ b/paimon-format/src/main/java/org/apache/paimon/format/parquet/reader/VectorizedDeltaByteArrayReader.java @@ -93,11 +93,11 @@ private void readValues(int total, WritableBytesVector c, int rowId) { c.putByteArray(rowId + i, bytes, offset, length); // Keep the value we just assembled rather than a view over the output vector's - // buffer. The record reader resets that vector between batches, and reset() zeroes - // the buffer, so a view into it would hand NUL bytes to the next value's prefix - // whenever a page spans more than one batch. skipBinary avoids the same hazard by - // alternating two vectors; here the array is already a fresh copy, so wrapping it - // costs nothing. + // buffer. The record reader reuses that vector across batches, rewriting it + // from offset 0, so a view into it would hand the next batch's bytes to the + // next value's prefix whenever a page spans more than one batch. skipBinary + // avoids the same hazard by alternating two vectors; here the array is + // already a fresh copy, so wrapping it costs nothing. previous = ByteBuffer.wrap(bytes); currentRow++; } diff --git a/paimon-format/src/test/java/org/apache/paimon/format/parquet/reader/DeltaByteArrayEncodingTest.java b/paimon-format/src/test/java/org/apache/paimon/format/parquet/reader/DeltaByteArrayEncodingTest.java index f1330f2f9cd0..f21e6db87016 100644 --- a/paimon-format/src/test/java/org/apache/paimon/format/parquet/reader/DeltaByteArrayEncodingTest.java +++ b/paimon-format/src/test/java/org/apache/paimon/format/parquet/reader/DeltaByteArrayEncodingTest.java @@ -102,6 +102,23 @@ public void testNegativeSize() { Integer.MAX_VALUE - 1)); } + /** + * skipBinary alternates two vectors and leaves {@code previous} pointing into the buffer of + * whichever one it wrote last, so after an odd number of skipped values the next skip call + * starts by resetting that very vector. The prefix of the following value is copied out of it. + */ + @Test + public void skippingAnOddNumberOfValuesKeepsThePrefix() throws Exception { + String[] vals = new String[] {"aaaa", "aaab", "aaac", "aaad"}; + Utils.writeData(writer, vals); + reader.initFromPage(vals.length, writer.getBytes().toInputStream()); + + reader.skipBinary(1); + reader.skipBinary(1); + + assertArrayEquals(vals[2].getBytes(), reader.readBinary(0).getBytes()); + } + /** * The record reader resets the vector between batches, so a page that spans two batches has to * survive that reset. Every other case here reads a whole page into one vector without a reset,