Skip to content

[common] Do not zero the HeapBytesVector buffer on reset - #9644

Open
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/heapbytesvector-reset-fill
Open

[common] Do not zero the HeapBytesVector buffer on reset#9644
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/heapbytesvector-reset-fill

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

Purpose

close #9643

HeapBytesVector.reset() zeroed its data buffer, one line under the comment saying it does not:

// We don't reset buffer to avoid unnecessary copy.
Arrays.fill(buffer, (byte) 0);

Beyond costing O(buffer size) per batch on the vectorized read path, it silently corrupts values. VectorizedDeltaByteArrayReader.skipBinary alternates two vectors and leaves previous pointing at the buffer of whichever one it wrote last, so after an odd number of skipped values previous points into tempBinaryValVector and the next skipBinary call opens by resetting that same vector. The prefix it then copies out of previous is all zeros, and the value that follows the skipped range comes back with NUL bytes in front.

Removing the fill is safe: every read goes through getBytes(i), bounded by the start and length arrays that reset() still clears, and null positions have length 0. Nothing in the repo reads the buffer directly except the two Parquet readers and ColumnVectorUtils, and since #9275 readValues keeps its own copy rather than a view into the vector.

The comment in VectorizedDeltaByteArrayReader that explained the copy in terms of "reset() zeroes the buffer" is updated to say what is actually true now: the vector is rewritten across batches, so a view into it cannot be kept.

Tests

DeltaByteArrayEncodingTest.skippingAnOddNumberOfValuesKeepsThePrefix writes four values sharing a prefix, calls skipBinary(1) twice so the second call resets the vector previous points into, and reads the next value.

Against the unfixed HeapBytesVector it fails with array contents differ at index [0], expected: <97> but was: <0>, which is the prefix that was zeroed. The existing randomStringsWithSkip and randomStringsWithSkipN never reach it, since they skip once.

HeapBytesVectorReserveBytesTest.testResetDoesNotWipeBuffer pins the vector's own behavior.

mvn -pl paimon-common,paimon-format -Dtest=HeapBytesVectorReserveBytesTest,DeltaByteArrayEncodingTest test on JDK 8: 13 and 8 tests, 0 failures. spotless:check and checkstyle:check on both modules are clean. The red run needs the pre-fix paimon-common installed, since the two modules do not share a reactor for this.

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.
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.

[Bug] Skipping an odd number of DELTA_BYTE_ARRAY values corrupts the next value prefix

1 participant