GH-36388: [C++][Python] Return error from MakeArrayFromScalar on offset overflow - #50024
Open
Sriniketh24 wants to merge 2 commits into
Open
GH-36388: [C++][Python] Return error from MakeArrayFromScalar on offset overflow#50024Sriniketh24 wants to merge 2 commits into
Sriniketh24 wants to merge 2 commits into
Conversation
…n offset overflow MakeArrayFromScalar silently created an invalid array with negative offsets when the total data size (value_size * repetition_count) exceeded the maximum value of the offset type. For 32-bit offset types like StringType and BinaryType, this threshold is INT32_MAX (~2 GB). The root cause was in CreateOffsetsBuffer where the running offset accumulated via OffsetType addition without checking for overflow, wrapping around to negative values. Added an early overflow check in CreateOffsetsBuffer that computes the total size in int64_t and compares against the offset type's maximum. On overflow, a Status::Invalid error is returned with a message suggesting the use of large_* types. This is AI-assisted work by Claude.
github-actions
Bot
added
Component: C++
Component: Python
awaiting review
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses GH-36388 by preventing MakeArrayFromScalar (and therefore pyarrow.repeat) from silently constructing invalid binary/string arrays when the repeated total byte size would exceed the maximum representable value of 32-bit offsets, returning an Invalid status with a clearer error instead.
Changes:
- Added an offset overflow check in
RepeatedArrayFactory::CreateOffsetsBufferfor variable-size offset types. - Added a C++ regression test covering string/binary offset overflow cases.
- Added a Python regression test verifying
pa.repeatraisesArrowInvalidon overflow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/pyarrow/tests/test_array.py | Adds Python coverage ensuring pa.repeat raises on 32-bit offset overflow. |
| cpp/src/arrow/array/util.cc | Introduces early offset overflow detection when creating offsets for repeated variable-size values. |
| cpp/src/arrow/array/array_test.cc | Adds a C++ regression test for MakeArrayFromScalar offset overflow behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+860
to
+864
| if (value_length > 0 && length_ > 0) { | ||
| int64_t total_size = static_cast<int64_t>(value_length) * length_; | ||
| if (total_size > static_cast<int64_t>(std::numeric_limits<OffsetType>::max())) { | ||
| return Status::Invalid( | ||
| "Cannot create array: total data size (", total_size, |
Comment on lines
+856
to
+860
| // Check that the total data size does not overflow the offset type. | ||
| // For 32-bit offset types (e.g. StringType, BinaryType), value_length * length_ | ||
| // must fit in int32_t, otherwise the offsets wrap around and produce an invalid | ||
| // array with negative offsets. | ||
| if (value_length > 0 && length_ > 0) { |
AlenkaF
requested changes
May 27, 2026
AlenkaF
left a comment
AlenkaF
left a comment
Member
There was a problem hiding this comment.
@Sriniketh24 I think it would be worth looking at the work already done in #38504 and continue from there.
… length checks Per @AlenkaF's suggestion to build on the approach from apache#38504: - Replace the manual int64_t multiplication (which could itself silently overflow for 64-bit offset types like large_string/large_binary) with arrow::internal::MultiplyWithOverflow, which is correct for both 32-bit and 64-bit OffsetType. - Reject length > numeric_limits<OffsetType>::max() up front, independent of value size (e.g. an empty string repeated too many times). - Reject negative length in MakeArrayFromScalar itself. - Add C++ test cases for the length-exceeds-offset-type and negative-length paths, on top of the existing overflow tests and the Python-level pa.repeat() regression test. Credit to @llama90's work in apache#38504, which reviewer @js8544 had already validated this approach on before it went stale.
Author
|
Hi @AlenkaF — thanks for the pointer to #38504, that was a good approach that just ran out of the original author's time (credit to @llama90 for the initial work, and @js8544 for reviewing it there). Reworked this PR to build on that approach:
Note: I wasn't able to run the full C++ test suite in my local environment (no existing build directory / ninja), so I only syntax-checked the changed file directly against the local headers (clean, no errors). CI here will be the real validation — flagging that up front rather than claiming more confidence than I have. |
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
pyarrow.repeat(backed byMakeArrayFromScalarin C++) silently created an invalid array with negative offsets when the total data size (value_size * repetition_count) exceededINT32_MAXfor 32-bit offset types (StringType,BinaryType). The resulting array passed creation without error but failed validation with a cryptic "Negative offsets in binary array" or "non-monotonic offset" message.What changed
Added an early overflow check in
RepeatedArrayFactory::CreateOffsetsBufferthat computes the total data size inint64_tand returnsStatus::Invalidwith an actionable error message when it would exceed the offset type's maximum. The error message suggests usinglarge_*types (e.g.large_string,large_binary) for data exceeding 2 GB.Are these changes tested?
Yes.
TestMakeArrayFromScalarOffsetOverflowinarray_test.cc— tests string, binary, and large_string scalarstest_repeat_offset_overflowintest_array.py— verifiespa.repeatraisesArrowInvalidon overflowAre there any user-facing changes?
Yes.
MakeArrayFromScalar(andpyarrow.repeat) now raisesArrowInvalidearly with a clear error message instead of silently returning a corrupt array. This is a strictly better user experience.Closes: #36388
This is AI-assisted work by Claude.