Repository navigation
GH-54885: [C++][Compute] Use 64-bit offsets for basic row encoding - #55170
Open
zanmato1984 wants to merge 1 commit into
Open
zanmato1984 wants to merge 1 commit into
zanmato1984 wants to merge 1 commit into
Conversation
Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
|
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Static review found no blocking issues in the consistent widening or regression coverage; the opt-in large-memory tests were not executed.
No open issues 🎉
What changed in this PR
Fixes the 2 GiB encoded-buffer overflow behind GH-54885 by widening internal byte offsets and lengths to 64 bits in basic row encoding and grouping. Row IDs, group IDs, and per-column encoding formats remain unchanged.
Changes:
- Widens RowEncoder offsets and shared KeyEncoder length counters.
- Updates basic Grouper temporary and persistent buffer offsets consistently.
- Adds six opt-in large-memory regressions covering encoders, grouping, and join keys and payloads.
| File | Description |
|---|---|
| cpp/src/arrow/compute/row/row_encoder_internal.h | Widens encoder interfaces, row lengths, and stored offsets. |
| cpp/src/arrow/compute/row/row_encoder_internal.cc | Uses 64-bit length calculations and cumulative append sizes. |
| cpp/src/arrow/compute/row/row_encoder_internal_test.cc | Tests large value lengths and cumulative appends beyond 2 GiB. |
| cpp/src/arrow/compute/row/grouper.cc | Widens batch and persistent unique-key offsets. |
| cpp/src/arrow/compute/row/grouper_test.cc | Tests batch and unique-key buffers beyond 2 GiB. |
| cpp/src/arrow/acero/hash_join_node_test.cc | Tests oversized build-side key and payload buffers. |
Review effort: Balanced
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
Basic hash joins store encoded build-side keys and payloads in contiguous RowEncoder buffers. Signed 32-bit offsets and byte counts overflow when the accumulated encoded data exceeds 2 GiB, even when the individual input arrays are valid. This can cause a vector length exception and terminate the calling Python process.
For large_string keys, each encoded value includes a one-byte null marker and an eight-byte length prefix, explaining why the failure can occur before the raw string data reaches 2 GiB.
What changes are included in this PR?
Row IDs, group IDs, and per-column encoded length formats are unchanged.
Are these changes tested?
Added six opt-in large-memory regression tests covering encoder length calculation and cumulative appends, basic Grouper batch and unique-key buffers, and hash-join keys and payloads.
Are there any user-facing changes?
Basic hash joins using large_string or large_binary columns can accumulate more than 2 GiB of encoded build-side keys or payloads without overflowing their cumulative byte offsets.
The memory cost is in the offset arrays: each offset grows from 4 to 8 bytes. A RowEncoder storing N rows has N + 1 offsets, so their element storage increases by 4 × (N + 1) bytes—approximately 3.8 MiB per million rows, per encoder, excluding spare vector capacity and allocator overhead. Separate key and payload encoders each incur this cost. This doubles offset-array storage, not total join memory.
Basic Grouper likewise uses four additional bytes per persistent unique-key offset and per temporary batch offset. The encoded key/value bytes and per-column length prefixes are unchanged. There are no public API changes, and existing row-count and group-count limits remain unchanged.
This PR contains a "Critical Fix". It addresses a crash on valid input when encoded build-side data exceeds the signed 32-bit offset range.
Was AI used for this PR?
The author conducted a thorough review with AI assistance.
PR code and description written by:
Reviewed before submission by:
large_stringkeys near 2gb #54885