zeroshade commented on code in PR #1490:
URL: https://github.com/apache/arrow-go/pull/1490#discussion_r4527600218
##########
arrow/compute/exec/span.go:
##########
@@ -327,6 +327,22 @@ func (a *ArraySpan) FillFromScalar(val scalar.Scalar) {
int64(len(dataBuffer)), 1)
}
a.Buffers[2].Buf = dataBuffer
+ case typeID == arrow.BINARY_VIEW || typeID == arrow.STRING_VIEW:
+ sc := val.(scalar.BinaryScalar)
+ a.Buffers[1].Buf = arrow.Uint64Traits.CastToBytes(a.Scratch[:])
+ a.Buffers[1].Owner = nil
+ a.Buffers[1].SelfAlloc = false
+ a.Buffers[2].Buf, a.Buffers[2].Owner = nil, nil
Review Comment:
For an inline value (<= 12 bytes) `Buffers[2]` stays empty, but
`getNumBuffers` returns 3 for view types, so `span.MakeArray()` produces
`[validity, headers, nil]`. `ValidateFull` accepts that, but
`array.Concatenate` panics: `concat.go` iterates `d.Buffers()[2:]` and calls
`buf.Retain()` on the nil buffer.
Repro:
```go
var s exec.ArraySpan
s.FillFromScalar(scalar.NewStringViewScalar("hi"))
a := s.MakeArray()
_, err := array.Concatenate([]arrow.Array{a, a}, memory.DefaultAllocator)
// arrow/concat: runtime error: invalid memory address or nil pointer
dereference
```
Same result mixing an inline and an out-of-line scalar span; two out-of-line
spans concatenate fine.
The underlying problem already exists in `ArraySpan.MakeData` (an
inline-only view array round-tripped through `SetMembers`/`MakeArray` fails the
same way), but with this change every inline view scalar hits it. Could
`MakeData` drop an empty trailing data buffer for `BINARY_VIEW`/`STRING_VIEW`
(matching what `MakeArrayFromScalar` does here), with a `Concatenate` check
added to `TestArraySpan_FillFromScalarRoundTripsViews`?
##########
arrow/scalar/append.go:
##########
@@ -57,10 +58,11 @@ func appendPrimitive[T primitives, B builder[T]](bldr B,
scalars []Scalar) {
}
func appendBinary(bldr binaryBuilder, scalars []Scalar) {
+ _, view := bldr.Type().(arrow.BinaryViewDataType)
var dataSize int
for _, s := range scalars {
s := s.(BinaryScalar)
- if s.IsValid() {
+ if s.IsValid() && (!view || !arrow.IsViewInline(len(s.Data())))
{
Review Comment:
For view builders, `dataSize` is the sum of all out-of-line values, and
`BinaryViewBuilder.ReserveData` runs it through `checkBinaryViewValueSize`,
which panics above 2 GiB. That limit is per value, not per array, so
`AppendSlice` panics on a set of view scalars whose combined out-of-line bytes
exceed 2 GiB, even though appending them individually works (`Append` reserves
per value and starts new blocks).
Suggest skipping the aggregate reserve for view builders and calling
`Append` per value instead of `UnsafeAppend`.
##########
arrow/scalar/scalar.go:
##########
@@ -834,10 +845,32 @@ func MakeArrayFromScalar(sc Scalar, length int, mem
memory.Allocator) (arrow.Arr
memory.Set(data.Bytes(), c)
return array.NewBoolean(length, data, nil, 0), nil
case BinaryScalar:
- if s.DataType().ID() == arrow.FIXED_SIZE_BINARY {
+ switch s.DataType().ID() {
+ case arrow.FIXED_SIZE_BINARY:
data := finishFixedWidth(s.Data())
defer data.Release()
return array.MakeFromData(data), nil
+ case arrow.BINARY_VIEW, arrow.STRING_VIEW:
+ value := s.Data()
+ if len(value) > math.MaxInt32 {
Review Comment:
nit: this guard exists only here. `FillFromScalar` and the scalars'
`Validate` don't check it, so an oversized value would overflow the int32 size
field in the view header there. Putting the check in `BinaryView`/`StringView`
`Validate` would cover every path.
##########
arrow/compute/scalar_nested.go:
##########
@@ -94,8 +94,7 @@ type listElementFunction struct {
func listElementScalarResultSupported(typ arrow.DataType, nullResult bool)
bool {
switch typ.ID() {
- case arrow.BINARY_VIEW, arrow.STRING_VIEW, arrow.LIST_VIEW,
arrow.LARGE_LIST_VIEW,
- arrow.DECIMAL32, arrow.DECIMAL64:
+ case arrow.LIST_VIEW, arrow.LARGE_LIST_VIEW, arrow.DECIMAL32,
arrow.DECIMAL64:
Review Comment:
nit: this doesn't change behavior. `validateListElementOutputType` →
`kernels.ListElementOutputTypeSupported` still rejects view types at any
nesting depth before these checks run (consistent with the PR description and
`listElementDoc`). Either drop these two hunks or note in the description that
they're cleanup only.
##########
arrow/scalar/scalar.go:
##########
@@ -675,6 +678,14 @@ func GetScalar(arr arrow.Array, idx int) (Scalar, error) {
return NewStringScalar(arr.Value(idx)), nil
case *array.LargeString:
return NewLargeStringScalar(arr.Value(idx)), nil
+ case *array.StringView:
+ return NewStringViewScalar(arr.Value(idx)), nil
+ case *array.BinaryView:
+ // An inline value lives in the view header, so copy rather than
+ // slice a data buffer.
Review Comment:
nit: this reads as if only inline values are copied, but every value is.
Maybe something like "Copy so the scalar shares neither the header nor the data
buffers of the array."
--
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]
