Repository navigation
Conversation
… pool tracking and Arrow string batching Closes apache#55487 Generated-by: Gemini (reviewed by Ryan Vickramasinghe)
|
|
|
|
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.
Rationale for this change
Fixes #55487.
DeltaLengthByteArrayEncodercurrently:MemoryPool*arguments in its constructor (pool = ::arrow::default_memory_pool()instead of a default parameter), causing bothDELTA_LENGTH_BYTE_ARRAYandDELTA_BYTE_ARRAY'ssuffix_encoder_to allocate ondefault_memory_pool()instead of the caller's pool.PutBinaryArraywithout pre-reservingsink_capacity, adding unnecessary per-value overhead and reallocations compared toPut(const ByteArray*, int)and other Parquet writers (such asparquet-rsand Polars).BufferBuilder::UnsafeAppend(nullptr, 0)on emptyByteArray(0, nullptr)values, triggering undefined behavior (memcpywith null source pointer) under UBSan.What changes are included in this PR?
DeltaLengthByteArrayEncoder's constructor signature to useMemoryPool* pool = ::arrow::default_memory_pool()as a default argument rather than reassigningpoolin the member initializer list.PutBinaryArray, pre-reservesink_capacity, bulk-copy contiguous non-null offset-basedBinaryArray/LargeBinaryArraypayloads in a singleUnsafeAppend, and skip zero-byte appends.Put(const ByteArray*, int)against callingUnsafeAppendon zero-length items.Are these changes tested?
Yes, added unit tests in
cpp/src/parquet/encoding_test.cc:DeltaLengthByteArrayEncodingAdHoc.UsesProvidedMemoryPoolverifying allocations acrossStringArray, nullableStringArray, slicedLargeStringArray,StringViewArray, rawByteArray, andDELTA_BYTE_ARRAYare tracked on a caller-suppliedProxyMemoryPool.DeltaLengthByteArrayEncodingAdHoc.BatchedLengthsSlicesAndZeroLengthSafetycovering 256-element batch boundaries (0, 1, 255, 256, 257, 512, 600), non-zero slice offsets onLargeStringArray,StringViewArray, empty/all-null arrays, andByteArray(0, nullptr)inputs.Are there any user-facing changes?
Callers passing a custom
MemoryPool*when writingDELTA_LENGTH_BYTE_ARRAYorDELTA_BYTE_ARRAYcolumns will now have encoder buffer allocations tracked on that pool.Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
AI coding assistance was used to help draft the
PutBinaryArraybatching implementation, unit tests, and PR description; all changes were reviewed and verified by the author.PR code and description written by:
Reviewed before submission by: