fix(arreflect): validate primitive conversion assignment overflow checks#903
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Two minor, non-blocking nits on the new FLOAT32 handling — the change itself is a solid defensive improvement and the tests are good.
| if v.OverflowFloat(val) { | ||
| return fmt.Errorf("cannot set float32 value %f into %s: %w", val, v.Type(), ErrTypeMismatch) | ||
| } | ||
| v.SetFloat(float64(arr.(*array.Float32).Value(i))) |
There was a problem hiding this comment.
Minor/consistency: this re-reads arr.(*array.Float32).Value(i) instead of reusing the val computed just above (every other branch uses v.SetInt(val)/v.SetUint(val)/v.SetFloat(val)). Same result, just tidier — v.SetFloat(val).
| return fmt.Errorf("cannot set float32 into %s: %w", v.Type(), ErrTypeMismatch) | ||
| } | ||
| val := float64(arr.(*array.Float32).Value(i)) | ||
| if v.OverflowFloat(val) { |
There was a problem hiding this comment.
Minor: for a FLOAT32 source this OverflowFloat check can never trigger — a float32 value widened to float64 always fits a float32 (or float64) destination, so it's effectively dead here, unlike the FLOAT64→float32 case below where it does real work. Harmless; feel free to drop it or leave a one-line note that it's kept for symmetry.
2 participants
Summary
Prevent silent corruption in
arreflectprimitive conversions by validating numeric assignment bounds before setting through reflection.Why this fix
setPrimitiveValueusedSetInt/SetUint/SetFloatdirectly for Arrow numeric types. That allowed values outside the target Go type range to wrap/truncate, so bad data could pass through without errors until consumed by fast paths relying on value integrity.Changes
OverflowIntOverflowUintOverflowFloatErrTypeMismatch-wrapped errors on overflow.Files
arrow/array/arreflect/reflect_arrow_to_go.goarrow/array/arreflect/reflect_arrow_to_go_test.goValidation
int64->int8uint64->uint8float64->float32Bug details