zeroshade commented on code in PR #1021:
URL: https://github.com/apache/arrow-go/pull/1021#discussion_r3666503144
##########
arrow/array/struct.go:
##########
@@ -143,6 +143,18 @@ func NewStructData(data arrow.ArrayData) *Struct {
func (a *Struct) NumField() int { return len(a.fields) }
func (a *Struct) Field(i int) arrow.Array { return a.fields[i] }
+func (a *Struct) Validate() error { return nil }
Review Comment:
This no-op `Validate()` is the first half of the problem. The two comparable
types in this package both do the cheap structural check here rather than
deferring everything to `ValidateFull`:
- `union.go:155-162` (sparse union) checks `fieldData.Len() <
a.data.length+a.data.offset` in `Validate()`
- `validate.go:404-412` (fixed-size list) runs its child-length check in
**both** paths
Since the check is O(number of fields) — not O(length) — there's no
performance reason to keep it out of `Validate()`.
##########
arrow/array/struct.go:
##########
@@ -143,6 +143,18 @@ func NewStructData(data arrow.ArrayData) *Struct {
func (a *Struct) NumField() int { return len(a.fields) }
func (a *Struct) Field(i int) arrow.Array { return a.fields[i] }
+func (a *Struct) Validate() error { return nil }
+
+func (a *Struct) ValidateFull() error {
+ for i, field := range a.fields {
+ if field.Len() < a.Len() {
+ return fmt.Errorf("%w: arrow/array: struct field %d
length %d is smaller than struct length %d",
+ arrow.ErrInvalid, i, field.Len(), a.Len())
Review Comment:
This compares the **post-`setData`** field length against `a.Len()`. Because
the fallback below replaces an out-of-range slice with the whole child, a
sliced corrupt struct ends up with `field.Len() == a.Len()` and passes
validation.
Compare against the raw child data and the full extent instead, as
`union.go:155` does:
```go
if a.data.childData[i].Len() < a.data.length+a.data.offset {
```
That is offset-aware and can't be defeated by slicing.
##########
arrow/array/struct.go:
##########
@@ -222,9 +234,14 @@ func (a *Struct) setData(data *Data) {
a.fields = make([]arrow.Array, len(data.childData))
for i, child := range data.childData {
if data.offset != 0 || child.Len() != data.length {
- sub := NewSliceData(child, int64(data.offset),
int64(data.offset+data.length))
- a.fields[i] = MakeFromData(sub)
- sub.Release()
+ end := int64(data.offset) + int64(data.length)
+ if data.offset >= 0 && end >= int64(data.offset) && end
<= int64(child.Len()) {
+ sub := NewSliceData(child, int64(data.offset),
end)
+ a.fields[i] = MakeFromData(sub)
+ sub.Release()
+ } else {
+ a.fields[i] = MakeFromData(child)
+ }
Review Comment:
This is the second half. When the range is out of bounds, silently using the
**unsliced** child means the offset is discarded — so `Field(0).Value(0)`
returns `child[0]` where the caller asked for `child[offset]`. I measured
exactly that: a sliced struct at `offset=1` returned the value at index 0 while
reporting `field0.Len()==a.Len()`.
Avoiding the old `index out of range` panic is a reasonable goal, but
substituting different data is worse than either panicking or clamping. Either
drop this fallback (and let `Validate` reject the array), or clamp the bounds:
```go
sub := NewSliceData(child, min(int64(data.offset), int64(child.Len())),
min(end, int64(child.Len())))
```
Note `union.setData:138-140` keeps its eager slice and relies on
`Validate()` to reject bad geometry — that's the pattern I'd follow here.
--
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]
