Repository navigation
GH-54906: [C++][Compute] Fix ascii_upper/ascii_lower/ascii_swapcase when the first offset is not 0 - #54914
Open
JonSnow1807 wants to merge 1 commit into
Open
GH-54906: [C++][Compute] Fix ascii_upper/ascii_lower/ascii_swapcase when the first offset is not 0#54914JonSnow1807 wants to merge 1 commit into
JonSnow1807 wants to merge 1 commit into
Conversation
…e first offset is not 0 StringDataTransform reused the input offsets buffer whenever the array offset was 0, but it writes the transformed data to a new buffer starting at byte 0. When the first value offset is not 0 the reused offsets then pointed past the end of the output data and the result failed validation. Only reuse the offsets buffer when the first offset is 0 and rebase the offsets otherwise, as is already done for sliced inputs.
|
|
dmitry-chirkov-dremio
approved these changes
Oct 29, 2026
dmitry-chirkov-dremio
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Clean fix and for the clear test.
Could you update the PR description? This fixes a bug that produced invalid data, so please add the "Critical Fix" line from the template.
Author
|
Done, added the "Critical Fix" line to the description. Thanks for the review. |
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
StringDataTransformreuses the input offsets buffer wheneverinput.offset == 0, but it writes the transformed characters to a new data buffer starting at byte 0 (it allocatesoffsets[length] - offsets[0]bytes). When the first value offset is not 0 (a valid array,ValidateFull()accepts it) the reused offsets point past the end of the output data and the result is invalid:Offset invariant failure: offset for slot 1 out of bounds: 8 > 3. Reproduced with the issue's snippet forstringandlarge_string.What changes are included in this PR?
Only reuse the input offsets buffer when the first offset is 0. Otherwise fall into the existing branch that allocates new offsets and rebases them by
offsets[0], which is already what sliced inputs do. The common case (first offset 0, unsliced input) is unchanged and still reuses the buffer without copying.Are these changes tested?
Yes.
TestStringKernels.AsciiCaseTransformNonZeroFirstOffset(typed overstringandlarge_string) builds an array with offsets[5, 8, 8, 8, 10], a null, an empty string and a leading unused data prefix, checks it validates and equals["abC", null, "", "De"], then checks all three kernels on the full array and on slices with a non-zero array offset. Without the fix the test fails with the offset invariant failure above; with itarrow-compute-scalar-type-testpasses (350 tests, Debug + ASan/UBSan).Out of scope: for a length-0 input whose offsets buffer is absent, the reuse branch already copied
sizeof(offset_type)bytes from a null pointer before this change; this PR does not change that path.Are there any user-facing changes?
The three kernels now return valid arrays for such inputs. No API change.
This PR contains a "Critical Fix". Before this change the three kernels returned arrays whose reused offsets pointed past the new data buffer, i.e. invalid data, for any input whose first value offset is not 0.
AI usage: the fix and the test were written with an AI coding assistant (Claude Code) under my direction; I reviewed every line and ran the tests locally.
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.
PR code and description written by:
Reviewed before submission by:
Code and tests were drafted with an AI coding assistant and then reviewed, built and tested by me; the description is mine.