zeroshade commented on code in PR #1024:
URL: https://github.com/apache/arrow-go/pull/1024#discussion_r3660289286
##########
arrow/scalar/scalar.go:
##########
@@ -788,6 +788,10 @@ func MakeArrayOfNull(dt arrow.DataType, length int, mem
memory.Allocator) arrow.
// MakeArrayFromScalar returns an array filled with the scalar value repeated
length times.
// Not yet implemented for nested types such as Struct, List, extension and so
on.
func MakeArrayFromScalar(sc Scalar, length int, mem memory.Allocator)
(arrow.Array, error) {
+ if length < 0 {
+ return nil, fmt.Errorf("%w: array length must be non-negative,
got %d", arrow.ErrInvalid, length)
+ }
Review Comment:
Guard placement is right — it precedes both the `MakeArrayOfNull` branch
below and the valid-scalar buffer allocation, so it covers both paths with one
check, and it returns before any allocation occurs (which is why the
`mem.AssertSize(t, 0)` in your test holds).
Wrapping `arrow.ErrInvalid` is also the correct convention here, since
`MakeArrayFromScalar` already returns an error rather than panicking.
My only note is on the PR description: the branch immediately below this
(`if !sc.IsValid()`) is the null path, and on `main` that path does **not**
panic — it silently returns an array with a negative length. Worth stating
accurately in the description since it becomes the commit message.
--
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]
