Skip to content

fix(array): honor struct offsets in constructors and validity masks#922

Merged
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:fix/struct-array-offset-validity
Jul 14, 2026
Merged

fix(array): honor struct offsets in constructors and validity masks#922
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:fix/struct-array-offset-validity

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Struct convenience construction could retain the full child length after applying a non-zero offset, causing child slicing to run past the end. Parent validity masking also addressed logical row positions instead of the child bitmap's physical offset.

This change validates struct offsets, uses length-offset for the constructed array, applies the existing null-bitmap validation to the names-based constructor, and masks child validity at physical bitmap positions. It also sizes replacement bitmaps for leading child offsets.

Tests:

  • go test ./arrow/array -run 'Test(NewStructArrayWithNullsValidatesOffset|StructStringMasksParentValidityAtPhysicalOffset|StructArrayStringerMasksRequiredChildWithoutNullBitmap)'
  • go test ./arrow/array

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 11, 2026 07:33

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — NewStructArrayWithNulls used child.Len() instead of length-offset (leading to OOB child slicing), and the validity masking cleared logical bit positions instead of the child's physical offset. Both are fixed correctly (offset validation in both constructors, length-offset, physical-offset-aware masking with SetNullN(UnknownNullCount)). One test nit inline; possible follow-up: validate that a non-nil parent null bitmap is long enough for offset+len.

Comment thread arrow/array/struct.go
}

length := cols[0].Len()
if err := validateStructArrayOffset(offset, length); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing offset-validation test only exercises NewStructArrayWithNulls; since this fix also guards NewStructArrayWithFieldsAndNulls, please add a mirrored invalid-offset case for it.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing the mask at the child's physical offset is the right fix. One offset-sensitive edge case remains in the caller — inline.

Comment thread arrow/array/struct.go
@@ -169,17 +188,19 @@ func (a *Struct) String() string {
func (a *Struct) newStructFieldWithParentValidityMask(fieldIndex int) arrow.Array {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This masking fix is correct, but the caller in String() still guards it behind !bytes.Equal(structBitmap, v.NullBitmapBytes()) (around L171). When a sliced child's bitmap bytes happen to equal the parent's but their physical offsets differ, masking is skipped and Struct.String() can print a value where the parent struct is null. Please fold offset compatibility into that shortcut (e.g. also apply the mask when a.Offset() != v.Data().Offset()), and add a regression test where the bitmap bytes are identical but the offsets differ.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — all prior review feedback is addressed. LGTM.

@zeroshade zeroshade merged commit c58f7a6 into apache:main Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants