Repository navigation
GH-54907: [C++] Fix SparseUnionArray::Make for sliced type_ids arrays - #54938
Open
JonSnow1807 wants to merge 1 commit into
Open
JonSnow1807 wants to merge 1 commit into
JonSnow1807 wants to merge 1 commit into
Conversation
…arrays SparseUnionArray::Make used type_ids.offset() as the offset of the union array. The offset of a sparse union also applies to its children, so with a sliced type_ids array the union referred to child positions shifted by that offset, while Make still required the children to have the same length as type_ids. This made it impossible to build a valid sparse union from a sliced type_ids array. Slice the type ids buffer to the logical window of type_ids (zero-copy) and build the union with offset 0, so children keep corresponding to the logical positions of type_ids. Unsliced inputs keep using the original buffer.
|
|
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
1 participant
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
SparseUnionArray::Makeusedtype_ids.offset()as the offset of the union array. The offset of a sparse union also applies to its children, so with a slicedtype_idsarray the union referred to child positions shifted by that offset, whileMakestill required the children to have the same length astype_ids. As reported in GH-54907, this made it impossible to build a valid sparse union from a slicedtype_idsarray: children of lengthlen(type_ids)produce an invalid array, children of lengthoffset + len(type_ids)are rejected.What changes are included in this PR?
When
type_idsis sliced, its type ids buffer is sliced (zero-copy) to the logical window oftype_idsand the union is built with offset 0, so the children keep corresponding to the logical positions oftype_idsas the existing length check requires. Unsliced inputs keep using the original buffer, so nothing changes for them.Are these changes tested?
Yes.
TestSparseUnionArray.MakeFromSlicedTypeIdsbuilds a union from a slicedtype_idsarray (with plain and with sliced children) and checks the values; on main it fails withSparse union child array #0 has length smaller than expected for union array (3 < 4).arrow-array-test,arrow-scalar-test,arrow-ipc-read-write-testandarrow-compute-scalar-type-testpass locally (Debug, ASan/UBSan).Are there any user-facing changes?
A sparse union built from a sliced
type_idsarray is now valid and has offset 0 instead oftype_ids.offset().AI usage disclosure: Claude Code assisted with investigating the root cause, preparing the implementation, and developing the regression test. I reviewed the changes and am responsible for their correctness.