zeroshade commented on code in PR #930:
URL: https://github.com/apache/arrow-go/pull/930#discussion_r3572931682
##########
arrow/array/validate.go:
##########
@@ -33,32 +36,401 @@ type Validator interface {
}
// Validate performs a basic O(1) consistency check on arr, returning an error
-// if the array's internal buffers are inconsistent. For array types that do
not
-// implement Validator, nil is returned.
+// if the array's internal buffers or nested child data are inconsistent.
//
// Use this to detect corrupted data from untrusted sources such as Arrow
Flight
// or Flight SQL servers before accessing values, which may otherwise panic.
func Validate(arr arrow.Array) error {
- if v, ok := arr.(Validator); ok {
- return v.Validate()
- }
- return nil
+ return validateArray(arr, false, "")
}
// ValidateFull performs a thorough O(n) consistency check on arr, returning an
-// error if the array's internal buffers are inconsistent. For array types that
-// do not implement Validator, nil is returned.
+// error if the array's internal buffers or nested child data are inconsistent.
//
// Unlike Validate, this checks every element and is therefore O(n). Use this
// when receiving data from untrusted sources where subtle corruption (e.g.
// non-monotonic offsets) may not be detected by Validate alone.
func ValidateFull(arr arrow.Array) error {
+ return validateArray(arr, true, "")
+}
+
+func validateArray(arr arrow.Array, full bool, path string) error {
+ if arr == nil {
+ return nil
+ }
+
+ data, ok := arr.Data().(*Data)
+ if !ok || data == nil {
+ return validationError(path, fmt.Errorf("arrow/array: array
does not expose internal data"))
+ }
+
+ if err := validateArrayData(data); err != nil {
+ return validationError(path, err)
+ }
+ if err := validateArrayStructure(data); err != nil {
+ return validationError(path, err)
+ }
+
if v, ok := arr.(Validator); ok {
- return v.ValidateFull()
+ var err error
+ if full {
+ err = v.ValidateFull()
+ } else {
+ err = v.Validate()
+ }
+ if err != nil {
+ return validationError(path, err)
+ }
+ }
+
+ if data.DataType().Layout().HasDict {
+ dictData := data.dictionary
+ if dictData != nil {
+ dt := data.DataType().(*arrow.DictionaryType)
+ indexData := NewData(dt.IndexType, data.length,
data.buffers, nil, data.nulls, data.offset)
+ err := checkIndexBounds(indexData,
uint64(dictData.Len()))
Review Comment:
`checkIndexBounds` runs from `validateArray` unconditionally, so it now
executes for basic `Validate` (`full == false`) as well. `Validate` is
documented as an O(1) layout check, but scanning every dictionary index makes
it O(n) — a real regression for large dictionary columns. Please gate the index
bounds scan behind `full` so it only runs in `ValidateFull`, keeping `Validate`
constant-time.
##########
arrow/array/validate.go:
##########
@@ -33,32 +36,401 @@ type Validator interface {
}
// Validate performs a basic O(1) consistency check on arr, returning an error
-// if the array's internal buffers are inconsistent. For array types that do
not
-// implement Validator, nil is returned.
+// if the array's internal buffers or nested child data are inconsistent.
//
// Use this to detect corrupted data from untrusted sources such as Arrow
Flight
// or Flight SQL servers before accessing values, which may otherwise panic.
func Validate(arr arrow.Array) error {
- if v, ok := arr.(Validator); ok {
- return v.Validate()
- }
- return nil
+ return validateArray(arr, false, "")
}
// ValidateFull performs a thorough O(n) consistency check on arr, returning an
-// error if the array's internal buffers are inconsistent. For array types that
-// do not implement Validator, nil is returned.
+// error if the array's internal buffers or nested child data are inconsistent.
//
// Unlike Validate, this checks every element and is therefore O(n). Use this
// when receiving data from untrusted sources where subtle corruption (e.g.
// non-monotonic offsets) may not be detected by Validate alone.
func ValidateFull(arr arrow.Array) error {
+ return validateArray(arr, true, "")
+}
+
+func validateArray(arr arrow.Array, full bool, path string) error {
+ if arr == nil {
+ return nil
+ }
+
+ data, ok := arr.Data().(*Data)
+ if !ok || data == nil {
+ return validationError(path, fmt.Errorf("arrow/array: array
does not expose internal data"))
+ }
+
+ if err := validateArrayData(data); err != nil {
+ return validationError(path, err)
+ }
+ if err := validateArrayStructure(data); err != nil {
+ return validationError(path, err)
+ }
+
if v, ok := arr.(Validator); ok {
- return v.ValidateFull()
+ var err error
+ if full {
+ err = v.ValidateFull()
+ } else {
+ err = v.Validate()
+ }
+ if err != nil {
+ return validationError(path, err)
+ }
+ }
+
+ if data.DataType().Layout().HasDict {
+ dictData := data.dictionary
+ if dictData != nil {
+ dt := data.DataType().(*arrow.DictionaryType)
+ indexData := NewData(dt.IndexType, data.length,
data.buffers, nil, data.nulls, data.offset)
+ err := checkIndexBounds(indexData,
uint64(dictData.Len()))
+ indexData.Release()
+ if err != nil {
+ return validationError(joinValidationPath(path,
"dictionary indices"), err)
+ }
+ }
+ }
+
+ if ext, ok := arr.(ExtensionArray); ok {
+ return validateArray(ext.Storage(), full,
joinValidationPath(path, "storage"))
+ }
+
+ for i, childData := range data.Children() {
+ child, err := makeArrayFromData(childData)
+ if err != nil {
+ return validationError(joinValidationPath(path,
validationChildPath(data.DataType(), i)), err)
+ }
+
+ childPath := joinValidationPath(path,
validationChildPath(data.DataType(), i))
+ err = validateArray(child, full, childPath)
+ child.Release()
+ if err != nil {
+ return err
+ }
+ }
+
+ if dictData := data.dictionary; dictData != nil {
+ dict, err := makeArrayFromData(dictData)
+ if err != nil {
+ return validationError(joinValidationPath(path,
"dictionary"), err)
+ }
+
+ err = validateArray(dict, full, joinValidationPath(path,
"dictionary"))
+ dict.Release()
+ if err != nil {
+ return err
+ }
+ }
+
+ return nil
+}
+
+func validateArrayData(data *Data) error {
+ if data == nil || data.dtype == nil {
+ return fmt.Errorf("arrow/array: array data has no data type")
+ }
+ if data.offset < 0 {
+ return fmt.Errorf("arrow/array: array offset is negative: %d",
data.offset)
+ }
+ if data.length < 0 {
+ return fmt.Errorf("arrow/array: array length is negative: %d",
data.length)
+ }
+ if data.nulls < UnknownNullCount || data.nulls > data.length {
+ return fmt.Errorf("arrow/array: invalid null count %d for
length %d", data.nulls, data.length)
+ }
+
+ end := int64(data.offset) + int64(data.length)
+ if end < int64(data.offset) {
+ return fmt.Errorf("arrow/array: array offset and length
overflow")
+ }
+
+ layout := data.dtype.Layout()
+ // Union arrays reserve the first buffer slot for a validity bitmap,
even
+ // though that slot must be nil and is not part of their type layout.
+ bufferOffset := 0
+ if data.dtype.ID() == arrow.SPARSE_UNION || data.dtype.ID() ==
arrow.DENSE_UNION {
+ bufferOffset = 1
+ }
+ bufferCount := len(data.buffers) - bufferOffset
+ if bufferCount < 0 {
+ bufferCount = 0
+ }
+ if bufferCount < len(layout.Buffers) {
+ return fmt.Errorf("arrow/array: expected at least %d buffers
for %s, got %d",
+ len(layout.Buffers), data.dtype, bufferCount)
+ }
+ if layout.VariadicSpec == nil && bufferCount > len(layout.Buffers) {
+ return fmt.Errorf("arrow/array: expected at most %d buffers for
%s, got %d",
+ len(layout.Buffers), data.dtype, bufferCount)
+ }
+
+ for i, spec := range layout.Buffers {
+ if err := validateBuffer(data.buffers[i+bufferOffset], spec,
end, data.length, i+bufferOffset); err != nil {
+ return err
+ }
+ }
+ if layout.VariadicSpec != nil {
+ for i := len(layout.Buffers); i < bufferCount; i++ {
+ if err := validateBuffer(data.buffers[i+bufferOffset],
*layout.VariadicSpec, end, data.length, i+bufferOffset); err != nil {
+ return err
+ }
+ }
}
return nil
}
+func validateBuffer(buf *memory.Buffer, spec arrow.BufferSpec, end int64,
length, index int) error {
+ if buf == nil {
+ if spec.Kind == arrow.KindFixedWidth || spec.Kind ==
arrow.KindVarWidth {
Review Comment:
This rejects a nil var-width **data** buffer whenever `length > 0`, but a
non-empty binary/string array can legitimately carry zero bytes of value data
(e.g. all-empty strings: offsets `[0, 0, …]` with a nil/empty data buffer).
`Binary.Validate`/`String.Validate` already allow this by checking the last
offset against `len(valueBytes)`, so this generic check will falsely reject
valid arrays before the type-specific validator runs. Please don't require
var-width data buffers to be non-nil based on `length` alone — let the
offset-derived length drive it (only require the buffer when the last offset is
> 0).
--
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]
