fix(array): validate run-end encoded invariants#992
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Can we add a test that goes through the array.Validate/array.ValidateFull functions also?
| runEnds := getRunEnds64(runEndsData) | ||
| lastRunEnd := runEnds[len(runEnds)-1] | ||
| if lastRunEnd < int64(r.data.offset+r.data.length) { | ||
| return fmt.Errorf("arrow/array: last run end is %d but it should cover %d", lastRunEnd, r.data.offset+r.data.length) | ||
| } | ||
|
|
||
| if !full { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
In the !full path, we're copying the entirety of the run ends only to read the single runEnds[len-1]. Instead for the !full path we should only check the last run end instead of doing the full copy loop
| case arrow.INT16: | ||
| runEnds := arrow.Int16Traits.CastFromBytes(data.Buffers()[1].Bytes()) | ||
| runEnds = runEnds[data.Offset() : data.Offset()+data.Len()] | ||
| out := make([]int64, len(runEnds)) | ||
| for i, v := range runEnds { | ||
| out[i] = int64(v) | ||
| } | ||
| return out | ||
| case arrow.INT32: | ||
| runEnds := arrow.Int32Traits.CastFromBytes(data.Buffers()[1].Bytes()) | ||
| runEnds = runEnds[data.Offset() : data.Offset()+data.Len()] | ||
| out := make([]int64, len(runEnds)) | ||
| for i, v := range runEnds { | ||
| out[i] = int64(v) | ||
| } | ||
| return out | ||
| default: | ||
| runEnds := arrow.Int64Traits.CastFromBytes(data.Buffers()[1].Bytes()) | ||
| return runEnds[data.Offset() : data.Offset()+data.Len()] | ||
| } |
There was a problem hiding this comment.
at this point we haven't validated the length/buffers yet, so we need to check len(runEnds) before you start slicing using data.Offset()+data.Len() otherwise we could panic on a corrupt buffer.
| func runEndTypeLimit(id arrow.Type) int64 { | ||
| switch id { | ||
| case arrow.INT16: | ||
| return 1<<15 - 1 |
| } | ||
| } | ||
|
|
||
| func getRunEnds64(data arrow.ArrayData) []int64 { |
There was a problem hiding this comment.
there's already a encoded.getRunEnds function at ree_utils.go:109 which returns a non-allocating closure, could we use export/reuse that pattern instead of allocating a new []int64?
2 participants
Summary
Why
Run-end encoded helpers already assume that run ends are strictly increasing and cover the logical range. Today malformed arrays can pass validation even when those invariants do not hold.
Validation