You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@parquet.apache.org by fo...@apache.org on 2026/08/25 19:04:26 UTC
(parquet-java) branch parquet-1.18.x created (now d64ae63a1)
This is an automated email from the ASF dual-hosted git repository.
Fokko pushed a change to branch parquet-1.18.x
in repository https://gitbox.apache.org/repos/asf/parquet-java.git
at d64ae63a1 Fix data corruption in ByteBufferBackedBinary.getBytes() for non-array-backed buffers (#3717)
This branch includes the following new commits:
new d64ae63a1 Fix data corruption in ByteBufferBackedBinary.getBytes() for non-array-backed buffers (#3717)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(parquet-java) 01/01: Fix data corruption in ByteBufferBackedBinary.getBytes() for non-array-backed buffers (#3717)
Posted by fo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
Fokko pushed a commit to branch parquet-1.18.x
in repository https://gitbox.apache.org/repos/asf/parquet-java.git
commit d64ae63a1d82490efa4fbec99f77b457facf7075
Author: Yiming Li <99...@users.noreply.github.com>
AuthorDate: Tue Aug 25 12:02:20 2026 -0700
Fix data corruption in ByteBufferBackedBinary.getBytes() for non-array-backed buffers (#3717)
FixedLenByteArrayPlainValuesReader hands out Binary values that all
share one page-wide ByteBuffer, advancing its live position on every
readBytes() call. getBytes() and toStringUsingUTF8() on the
non-array-backed path called value.limit(offset + length) directly on
that shared buffer before capturing position. ByteBuffer.limit()
clamps position down whenever position > newLimit, so calling
getBytes() on an earlier value after later values have already
advanced the buffer permanently rewinds its live position -- corrupting
every readBytes() call that follows.
This surfaces as data corruption when reading a repeated
(LIST) FIXED_LEN_BYTE_ARRAY column with 2+ elements per row across 2+
rows: record assembly stores each Binary and only materializes it once
a full row is built, which is exactly the lazy-after-later-value
pattern that triggers the clamp. Each subsequent row reads back the
previous row's last-written element instead of its own.
Fix both methods to duplicate() the buffer before adjusting its
position/limit, so the shared buffer's own position is never mutated.
Co-authored-by: Claude Sonnet 5 <no...@anthropic.com>
---
.../java/org/apache/parquet/io/api/Binary.java | 37 +++++++------
...stFixedLenByteArrayPlainValuesWriterReader.java | 60 ++++++++++++++++++++++
2 files changed, 80 insertions(+), 17 deletions(-)
diff --git a/parquet-column/src/main/java/org/apache/parquet/io/api/Binary.java b/parquet-column/src/main/java/org/apache/parquet/io/api/Binary.java
index 3160f091e..58961256f 100644
--- a/parquet-column/src/main/java/org/apache/parquet/io/api/Binary.java
+++ b/parquet-column/src/main/java/org/apache/parquet/io/api/Binary.java
@@ -442,16 +442,14 @@ public abstract class Binary implements Comparable<Binary>, Serializable {
if (value.hasArray()) {
ret = new String(value.array(), value.arrayOffset() + offset, length, StandardCharsets.UTF_8);
} else {
- int limit = value.limit();
- value.limit(offset + length);
- int position = value.position();
- value.position(offset);
- // no corresponding interface to read a subset of a buffer, would have to slice it
- // which creates another ByteBuffer object or do what is done here to adjust the
- // limit/offset and set them back after
- ret = StandardCharsets.UTF_8.decode(value).toString();
- value.limit(limit);
- value.position(position);
+ // Duplicate before adjusting position/limit so we never mutate the shared
+ // buffer's own position: readBytes() may have already advanced it past
+ // this value's range (e.g. lazily-consumed values in a repeated field),
+ // and limit(offset + length) would otherwise silently clamp it backwards.
+ ByteBuffer duplicate = value.duplicate();
+ duplicate.position(offset);
+ duplicate.limit(offset + length);
+ ret = StandardCharsets.UTF_8.decode(duplicate).toString();
}
return ret;
@@ -475,13 +473,18 @@ public abstract class Binary implements Comparable<Binary>, Serializable {
public byte[] getBytes() {
byte[] bytes = new byte[length];
- int limit = value.limit();
- value.limit(offset + length);
- int position = value.position();
- value.position(offset);
- value.get(bytes);
- value.limit(limit);
- value.position(position);
+ if (value.hasArray()) {
+ System.arraycopy(value.array(), value.arrayOffset() + offset, bytes, 0, length);
+ } else {
+ // Duplicate before adjusting position/limit so we never mutate the shared
+ // buffer's own position: readBytes() may have already advanced it past
+ // this value's range (e.g. lazily-consumed values in a repeated field),
+ // and limit(offset + length) would otherwise silently clamp it backwards.
+ ByteBuffer duplicate = value.duplicate();
+ duplicate.position(offset);
+ duplicate.limit(offset + length);
+ duplicate.get(bytes);
+ }
if (!isBackingBytesReused) { // backing buffer might change
cachedBytes = bytes;
}
diff --git a/parquet-column/src/test/java/org/apache/parquet/column/values/plain/TestFixedLenByteArrayPlainValuesWriterReader.java b/parquet-column/src/test/java/org/apache/parquet/column/values/plain/TestFixedLenByteArrayPlainValuesWriterReader.java
index d86eefbe7..66db13c10 100644
--- a/parquet-column/src/test/java/org/apache/parquet/column/values/plain/TestFixedLenByteArrayPlainValuesWriterReader.java
+++ b/parquet-column/src/test/java/org/apache/parquet/column/values/plain/TestFixedLenByteArrayPlainValuesWriterReader.java
@@ -160,4 +160,64 @@ public class TestFixedLenByteArrayPlainValuesWriterReader {
// Should not throw
}
}
+
+ // ---- Lazy getBytes() must not corrupt the shared page buffer's live position ----
+
+ // Regression test: getBytes() on an old value used to shift the buffer's shared
+ // read position backwards, so the next readBytes() call returned stale data.
+ @Test
+ public void testLazyGetBytesDoesNotCorruptSubsequentReadsDirectBuffer() throws IOException {
+ Binary[] expected = {fixedBinary(0), fixedBinary(50), fixedBinary(100), fixedBinary(150)};
+ ByteBuffer direct = writeToDirectBuffer(expected);
+
+ FixedLenByteArrayPlainValuesReader reader = new FixedLenByteArrayPlainValuesReader(FIXED_LEN);
+ reader.initFromPage(expected.length, ByteBufferInputStream.wrap(direct));
+
+ // "row 0": read two values, then materialize them lazily -- getBytes() on the
+ // first value happens after the shared buffer's position has already moved past it.
+ Binary row0v0 = reader.readBytes();
+ Binary row0v1 = reader.readBytes();
+ assertThat(row0v0.getBytes()).as("row0 value 0").isEqualTo(expected[0].getBytes());
+ assertThat(row0v1.getBytes()).as("row0 value 1").isEqualTo(expected[1].getBytes());
+
+ // "row 1": the corruption from materializing row 0 above must not affect this.
+ Binary row1v0 = reader.readBytes();
+ Binary row1v1 = reader.readBytes();
+ assertThat(row1v0.getBytes()).as("row1 value 0").isEqualTo(expected[2].getBytes());
+ assertThat(row1v1.getBytes()).as("row1 value 1").isEqualTo(expected[3].getBytes());
+ }
+
+ // Same scenario using toStringUsingUTF8(), which shares the buggy non-array-backed
+ // path with getBytes().
+ @Test
+ public void testLazyToStringUsingUTF8DoesNotCorruptSubsequentReadsDirectBuffer() throws IOException {
+ Binary[] expected = {fixedBinary(0), fixedBinary(50), fixedBinary(100), fixedBinary(150)};
+ ByteBuffer direct = writeToDirectBuffer(expected);
+
+ FixedLenByteArrayPlainValuesReader reader = new FixedLenByteArrayPlainValuesReader(FIXED_LEN);
+ reader.initFromPage(expected.length, ByteBufferInputStream.wrap(direct));
+
+ Binary row0v0 = reader.readBytes();
+ Binary row0v1 = reader.readBytes();
+ assertThat(row0v0.toStringUsingUTF8()).as("row0 value 0").isEqualTo(expected[0].toStringUsingUTF8());
+ assertThat(row0v1.toStringUsingUTF8()).as("row0 value 1").isEqualTo(expected[1].toStringUsingUTF8());
+
+ Binary row1v0 = reader.readBytes();
+ Binary row1v1 = reader.readBytes();
+ assertThat(row1v0.getBytes()).as("row1 value 0").isEqualTo(expected[2].getBytes());
+ assertThat(row1v1.getBytes()).as("row1 value 1").isEqualTo(expected[3].getBytes());
+ }
+
+ private ByteBuffer writeToDirectBuffer(Binary[] values) throws IOException {
+ try (FixedLenByteArrayPlainValuesWriter writer = newWriter()) {
+ for (Binary v : values) {
+ writer.writeBytes(v);
+ }
+ byte[] pageBytes = writer.getBytes().toByteArray();
+ ByteBuffer direct = ByteBuffer.allocateDirect(pageBytes.length);
+ direct.put(pageBytes);
+ direct.flip();
+ return direct;
+ }
+ }
}