zeroshade commented on code in PR #994:
URL: https://github.com/apache/arrow-go/pull/994#discussion_r3646743570
##########
arrow/array/binary.go:
##########
@@ -552,6 +563,86 @@ 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)
+ 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] {
Review Comment:
`gofmt` fails here (CI lint) — it wants spaces around `:` when the slice
bounds are expressions:
```suggestion
for _, b := range raw[4+view.Len() :
arrow.ViewHeaderSizeBytes] {
```
##########
arrow/array/validate_test.go:
##########
@@ -187,6 +216,71 @@ func TestLargeStringValidate(t *testing.T) {
})
}
+func TestBinaryViewValidate(t *testing.T) {
Review Comment:
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`.
##########
arrow/array/binary.go:
##########
@@ -552,6 +563,86 @@ 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)
+ rawViews := data.buffers[1].Bytes()
Review Comment:
**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):
```go
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.
```suggestion
if data.length == 0 {
return nil
}
rawViews := data.buffers[1].Bytes()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
