[common] Do not zero the HeapBytesVector buffer on reset - #9644
Open
LuciferYang wants to merge 1 commit into
Open
[common] Do not zero the HeapBytesVector buffer on reset#9644LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
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.
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.
Purpose
close #9643
HeapBytesVector.reset()zeroed its data buffer, one line under the comment saying it does not:Beyond costing O(buffer size) per batch on the vectorized read path, it silently corrupts values.
VectorizedDeltaByteArrayReader.skipBinaryalternates two vectors and leavespreviouspointing at the buffer of whichever one it wrote last, so after an odd number of skipped valuespreviouspoints intotempBinaryValVectorand the nextskipBinarycall opens by resetting that same vector. The prefix it then copies out ofpreviousis 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 thestartandlengtharrays thatreset()still clears, and null positions have length 0. Nothing in the repo reads the buffer directly except the two Parquet readers andColumnVectorUtils, and since #9275readValueskeeps its own copy rather than a view into the vector.The comment in
VectorizedDeltaByteArrayReaderthat 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.skippingAnOddNumberOfValuesKeepsThePrefixwrites four values sharing a prefix, callsskipBinary(1)twice so the second call resets the vectorpreviouspoints into, and reads the next value.Against the unfixed
HeapBytesVectorit fails witharray contents differ at index [0], expected: <97> but was: <0>, which is the prefix that was zeroed. The existingrandomStringsWithSkipandrandomStringsWithSkipNnever reach it, since they skip once.HeapBytesVectorReserveBytesTest.testResetDoesNotWipeBufferpins the vector's own behavior.mvn -pl paimon-common,paimon-format -Dtest=HeapBytesVectorReserveBytesTest,DeltaByteArrayEncodingTest teston JDK 8: 13 and 8 tests, 0 failures.spotless:checkandcheckstyle:checkon 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.