Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions arrow/array/binary.go
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,17 @@ func (a *BinaryView) ValueLen(i int) int {
return s.Len()
}

func (a *BinaryView) Validate() error {
return validateViewLayout(a, "binary view")
}

func (a *BinaryView) ValidateFull() error {
if err := a.Validate(); err != nil {
return err
}
return validateViewValues(a, a.dataBuffers, nil)
}

// ValueString returns the value at index i as a string instead of
// a byte slice, without copying the underlying data.
func (a *BinaryView) ValueString(i int) string {
Expand Down Expand Up @@ -552,6 +563,89 @@ func arrayEqualBinaryView(left, right *BinaryView) bool {
return true
}

func validateViewLayout(arr ViewLike, kind string) error {
data := arr.Data().(*Data)
if data.length == 0 {
return nil
}
if data.buffers[1] == nil {
return fmt.Errorf("arrow/array: non-empty %s array has no view buffer", kind)
}

expNumViews := data.offset + data.length
if len(data.buffers[1].Bytes())/arrow.ViewHeaderSizeBytes < expNumViews {
return fmt.Errorf("arrow/array: %s buffer must have at least %d view values", kind, expNumViews)
}
return nil
}

func validateViewValues(arr ViewLike, dataBuffers []*memory.Buffer, validateValue func(int, []byte) error) error {
data := arr.Data().(*Data)
if data.length == 0 {
return nil
}
rawViews := data.buffers[1].Bytes()

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.

Blocking: ValidateFull panics on empty (length-0) view arrays.

Validate() (via validateViewLayout) returns early when length == 0 and never dereferences buffers[1], but for an empty view array buffers[1] is nil, so this hoisted data.buffers[1].Bytes() panics.

Reproduced on the standard builder output (and on hand-built data):

array.ValidateFull(array.NewBinaryViewBuilder(mem).NewBinaryViewArray()) // panic: invalid memory address or nil pointer dereference
array.ValidateFull(array.NewStringViewBuilder(mem).NewStringViewArray()) // panic

This regressed in the "tighten view validation helpers" commit — patch 1 only touched buffers[1] inside the loop, so empty arrays were safe. It also undercuts the helper's intent: validation should report malformed data, not crash on a valid empty array.

Suggested change
rawViews := data.buffers[1].Bytes()
if data.length == 0 {
return nil
}
rawViews := data.buffers[1].Bytes()

for i := 0; i < data.length; i++ {
if arr.IsNull(i) {
continue
}

view := arr.ValueHeader(i)
if view.Len() < 0 {
return fmt.Errorf("arrow/array: view at slot %d has negative size %d", i, view.Len())
}

if view.IsInline() {
rawOffset := (data.offset + i) * arrow.ViewHeaderSizeBytes
raw := rawViews[rawOffset : rawOffset+arrow.ViewHeaderSizeBytes]
for _, b := range raw[4+view.Len() : arrow.ViewHeaderSizeBytes] {
if b != 0 {
return fmt.Errorf("arrow/array: view at slot %d was inline with size %d but its padding bytes were not all zero", i, view.Len())
}
}
if validateValue != nil {
if err := validateValue(i, view.InlineBytes()); err != nil {
return err
}
}
continue
}

if view.BufferIndex() < 0 {
return fmt.Errorf("arrow/array: view at slot %d has negative buffer index %d", i, view.BufferIndex())
}
if view.BufferOffset() < 0 {
return fmt.Errorf("arrow/array: view at slot %d has negative offset %d", i, view.BufferOffset())
}
if int(view.BufferIndex()) >= len(dataBuffers) {
return fmt.Errorf("arrow/array: view at slot %d references buffer %d but there are only %d data buffers", i, view.BufferIndex(), len(dataBuffers))
}

buf := dataBuffers[view.BufferIndex()]
if buf == nil {
return fmt.Errorf("arrow/array: view at slot %d references nil data buffer %d", i, view.BufferIndex())
}

offset := int(view.BufferOffset())
end := offset + view.Len()
if end > buf.Len() {
return fmt.Errorf("arrow/array: view at slot %d references range %d-%d of buffer %d but that buffer is only %d bytes long", i, offset, end, view.BufferIndex(), buf.Len())
}

value := buf.Bytes()[offset:end]
prefix := view.Prefix()
if !bytes.Equal(value[:arrow.ViewPrefixLen], prefix[:]) {
return fmt.Errorf("arrow/array: view at slot %d has inlined prefix %x but the out-of-line data begins with %x", i, prefix, value[:arrow.ViewPrefixLen])
}
if validateValue != nil {
if err := validateValue(i, value); err != nil {
return err
}
}
}
return nil
}

var (
_ arrow.Array = (*Binary)(nil)
_ arrow.Array = (*LargeBinary)(nil)
Expand Down
16 changes: 16 additions & 0 deletions arrow/array/string.go
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,22 @@ func (a *StringView) ValueLen(i int) int {
return s.Len()
}

func (a *StringView) Validate() error {
return validateViewLayout(a, "string view")
}

func (a *StringView) ValidateFull() error {
if err := a.Validate(); err != nil {
return err
}
return validateViewValues(a, a.dataBuffers, func(i int, value []byte) error {
if !utf8.Valid(value) {
return fmt.Errorf("arrow/array: string view at slot %d is not valid utf8", i)
}
return nil
})
}

func (a *StringView) String() string {
var o strings.Builder
o.WriteString("[")
Expand Down
207 changes: 207 additions & 0 deletions arrow/array/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/apache/arrow-go/v18/arrow"
"github.com/apache/arrow-go/v18/arrow/bitutil"
"github.com/apache/arrow-go/v18/arrow/endian"
"github.com/apache/arrow-go/v18/arrow/memory"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -77,6 +78,42 @@ func makeInt32ArrayRaw(t *testing.T, values []int32, validity []byte, nulls, len
return arr
}

func makeBinaryViewArrayRaw(t *testing.T, headerBytes []byte, dataBuffers []*memory.Buffer, validity []byte, nulls, length, offset int) *BinaryView {
t.Helper()
var validityBuf *memory.Buffer
if validity != nil {
validityBuf = memory.NewBufferBytes(validity)
}
viewBuf := memory.NewBufferBytes(headerBytes)
buffers := append([]*memory.Buffer{validityBuf, viewBuf}, dataBuffers...)
data := NewData(arrow.BinaryTypes.BinaryView, length, buffers, nil, nulls, offset)
arr := NewBinaryViewData(data)
data.Release()
return arr
}

func makeStringViewArrayRaw(t *testing.T, headerBytes []byte, dataBuffers []*memory.Buffer, validity []byte, nulls, length, offset int) *StringView {
t.Helper()
var validityBuf *memory.Buffer
if validity != nil {
validityBuf = memory.NewBufferBytes(validity)
}
viewBuf := memory.NewBufferBytes(headerBytes)
buffers := append([]*memory.Buffer{validityBuf, viewBuf}, dataBuffers...)
data := NewData(arrow.BinaryTypes.StringView, length, buffers, nil, nulls, offset)
arr := NewStringViewData(data)
data.Release()
return arr
}

func setViewHeaderBufferIndex(raw []byte, idx int32) {
endian.Native.PutUint32(raw[8:12], uint32(idx))
}

func setViewHeaderOffset(raw []byte, offset int32) {
endian.Native.PutUint32(raw[12:16], uint32(offset))
}

func TestBinaryValidate(t *testing.T) {
t.Run("valid array passes", func(t *testing.T) {
// offsets [0,3,6,9], data "abcdefghi" — 3 elements of 3 bytes each
Expand Down Expand Up @@ -201,6 +238,176 @@ func TestLargeStringValidate(t *testing.T) {
})
}

func TestBinaryViewValidate(t *testing.T) {

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.

Nice regression set. A few high-value cases are missing — the first would have caught the empty-array panic in validateViewValues:

  • Empty (length-0) array via array.ValidateFull (currently panics).
  • Drive the public array.Validate / array.ValidateFull entrypoints, not just the methods directly, so framework dispatch + layout checks are covered.
  • An array with offset > 0 to lock in the (data.offset+i)*ViewHeaderSizeBytes indexing.
  • Out-of-line happy path plus untested failure modes: prefix mismatch, negative / out-of-range buffer offset, and offset+size > buffer length.

t.Run("empty arrays pass top level validation", func(t *testing.T) {
mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
defer mem.AssertSize(t, 0)

binaryArr := NewBinaryViewBuilder(mem).NewBinaryViewArray()
defer binaryArr.Release()
stringArr := NewStringViewBuilder(mem).NewStringViewArray()
defer stringArr.Release()

assert.NoError(t, Validate(binaryArr))
assert.NoError(t, ValidateFull(binaryArr))
assert.NoError(t, Validate(stringArr))
assert.NoError(t, ValidateFull(stringArr))
})

t.Run("valid array passes", func(t *testing.T) {
var headers [1]arrow.ViewHeader
headers[0].SetBytes([]byte("hello"))
arr := makeBinaryViewArrayRaw(t, arrow.ViewHeaderTraits.CastToBytes(headers[:]), nil, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, arr.Validate())
assert.NoError(t, arr.ValidateFull())
})

t.Run("out of line values pass top level validation", func(t *testing.T) {
value := []byte("this payload is out of line")
var headers [1]arrow.ViewHeader
headers[0].SetBytes(value)
headers[0].SetIndexOffset(0, 0)
dataBuf := memory.NewBufferBytes(value)
arr := makeBinaryViewArrayRaw(t, arrow.ViewHeaderTraits.CastToBytes(headers[:]), []*memory.Buffer{dataBuf}, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, Validate(arr))
assert.NoError(t, ValidateFull(arr))
})

t.Run("offset arrays use the correct view slot", func(t *testing.T) {
headers := [2]arrow.ViewHeader{}
headers[0].SetBytes([]byte("skip"))
headers[1].SetBytes([]byte("keep"))
arr := makeBinaryViewArrayRaw(t, arrow.ViewHeaderTraits.CastToBytes(headers[:]), nil, nil, 0, 1, 1)
defer arr.Release()

assert.NoError(t, Validate(arr))
assert.NoError(t, ValidateFull(arr))
})

t.Run("negative size passes Validate but fails ValidateFull", func(t *testing.T) {
headerBytes := make([]byte, arrow.ViewHeaderSizeBytes)
endian.Native.PutUint32(headerBytes[:4], ^uint32(0))
arr := makeBinaryViewArrayRaw(t, headerBytes, nil, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, arr.Validate())
err := arr.ValidateFull()
require.Error(t, err)
assert.Contains(t, err.Error(), "negative size")
})

t.Run("missing referenced buffer passes Validate but fails ValidateFull", func(t *testing.T) {
var headers [1]arrow.ViewHeader
headers[0].SetBytes([]byte("this is longer than twelve"))
headers[0].SetIndexOffset(0, 0)
arr := makeBinaryViewArrayRaw(t, arrow.ViewHeaderTraits.CastToBytes(headers[:]), nil, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, arr.Validate())
err := arr.ValidateFull()
require.Error(t, err)
assert.Contains(t, err.Error(), "references buffer 0")
})

t.Run("prefix mismatch passes Validate but fails ValidateFull", func(t *testing.T) {
value := []byte("this payload is out of line")
var headers [1]arrow.ViewHeader
headers[0].SetBytes(value)
headers[0].SetIndexOffset(0, 0)
headerBytes := append([]byte(nil), arrow.ViewHeaderTraits.CastToBytes(headers[:])...)
headerBytes[4] ^= 0xff
dataBuf := memory.NewBufferBytes(value)
arr := makeBinaryViewArrayRaw(t, headerBytes, []*memory.Buffer{dataBuf}, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, Validate(arr))
err := ValidateFull(arr)
require.Error(t, err)
assert.Contains(t, err.Error(), "out-of-line data begins with")
})

t.Run("negative buffer offset passes Validate but fails ValidateFull", func(t *testing.T) {
value := []byte("this payload is out of line")
var headers [1]arrow.ViewHeader
headers[0].SetBytes(value)
headers[0].SetIndexOffset(0, 0)
headerBytes := append([]byte(nil), arrow.ViewHeaderTraits.CastToBytes(headers[:])...)
setViewHeaderOffset(headerBytes, -1)
dataBuf := memory.NewBufferBytes(value)
arr := makeBinaryViewArrayRaw(t, headerBytes, []*memory.Buffer{dataBuf}, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, Validate(arr))
err := ValidateFull(arr)
require.Error(t, err)
assert.Contains(t, err.Error(), "negative offset")
})

t.Run("negative buffer index passes Validate but fails ValidateFull", func(t *testing.T) {
value := []byte("this payload is out of line")
var headers [1]arrow.ViewHeader
headers[0].SetBytes(value)
headers[0].SetIndexOffset(0, 0)
headerBytes := append([]byte(nil), arrow.ViewHeaderTraits.CastToBytes(headers[:])...)
setViewHeaderBufferIndex(headerBytes, -1)
dataBuf := memory.NewBufferBytes(value)
arr := makeBinaryViewArrayRaw(t, headerBytes, []*memory.Buffer{dataBuf}, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, Validate(arr))
err := ValidateFull(arr)
require.Error(t, err)
assert.Contains(t, err.Error(), "negative buffer index")
})

t.Run("referenced range beyond buffer length passes Validate but fails ValidateFull", func(t *testing.T) {
value := []byte("this payload is out of line")
var headers [1]arrow.ViewHeader
headers[0].SetBytes(value)
headers[0].SetIndexOffset(0, 2)
dataBuf := memory.NewBufferBytes(value)
arr := makeBinaryViewArrayRaw(t, arrow.ViewHeaderTraits.CastToBytes(headers[:]), []*memory.Buffer{dataBuf}, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, Validate(arr))
err := ValidateFull(arr)
require.Error(t, err)
assert.Contains(t, err.Error(), "references range")
})

t.Run("inline padding bytes fail ValidateFull", func(t *testing.T) {
var headers [1]arrow.ViewHeader
headers[0].SetBytes([]byte("x"))
headerBytes := append([]byte(nil), arrow.ViewHeaderTraits.CastToBytes(headers[:])...)
headerBytes[8] = 1
arr := makeBinaryViewArrayRaw(t, headerBytes, nil, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, arr.Validate())
err := arr.ValidateFull()
require.Error(t, err)
assert.Contains(t, err.Error(), "padding bytes were not all zero")
})
}

func TestStringViewValidate(t *testing.T) {
t.Run("invalid utf8 passes Validate but fails ValidateFull", func(t *testing.T) {
var headers [1]arrow.ViewHeader
headers[0].SetBytes([]byte{0xff})
arr := makeStringViewArrayRaw(t, arrow.ViewHeaderTraits.CastToBytes(headers[:]), nil, nil, 0, 1, 0)
defer arr.Release()

assert.NoError(t, arr.Validate())
err := arr.ValidateFull()
require.Error(t, err)
assert.Contains(t, err.Error(), "not valid utf8")
})
}

func TestTopLevelValidate(t *testing.T) {
t.Run("Validate dispatches to Validator", func(t *testing.T) {
// non-monotonic string array: passes setData but ValidateFull must fail
Expand Down
Loading