zeroshade commented on code in PR #1323:
URL: https://github.com/apache/arrow-go/pull/1323#discussion_r4085829885
##########
arrow/array/concat.go:
##########
@@ -223,7 +244,7 @@ func handle32BitOffsetsData(data []arrow.ArrayData, out
*memory.Buffer, outLen i
buf := d.Buffers()[1]
if buf == nil {
- return nil, errors.New("array/concat: binary array is
missing an offset buffer")
+ return nil, errors.New("array/concat: array is missing
an offset buffer")
Review Comment:
This resolves my earlier comment — thanks.
For anyone reading later: the type-neutral wording is the right call here
because this helper is genuinely shared by four logical types (List, LargeList,
Binary, LargeBinary), so there's no single correct type name to print.
Threading the logical type through just to name it in an error wasn't worth the
parameter.
Worth being aware the change is larger than the wording for lists: on `main`
this branch wasn't reachable for List at all, because `gatherFixedBuffers`
skipped nil buffers upstream and the failure surfaced later as `index out of
range [0] with length 0`. So this is a new, earlier, typed failure rather than
a reworded one.
##########
arrow/array/concat.go:
##########
@@ -211,6 +211,27 @@ func concatBinaryBuffers(data []arrow.ArrayData,
byteWidth, length int, out *Dat
return nil
}
+func concatOffsetsData(data []arrow.ArrayData, byteWidth, length int, mem
memory.Allocator) (*memory.Buffer, []rng, error) {
+ out := memory.NewResizableBuffer(mem)
+ out.Resize(byteWidth * (length + 1))
+
+ var (
+ valueRanges []rng
+ err error
+ )
+ switch byteWidth {
+ case arrow.Int64SizeBytes:
+ valueRanges, err = handle64BitOffsetsData(data, out, length)
+ default:
+ valueRanges, err = handle32BitOffsetsData(data, out, length)
+ }
+ if err != nil {
+ out.Release()
+ return nil, nil, err
+ }
Review Comment:
This `out.Release()` is doing more than tidying up — it fixes a real leak,
and I don't think the PR description gives it credit.
On `main`, the List/LargeList nil-offset path allocates the output buffer in
`concatOffsets` and then dies via a recovered panic downstream, so the buffer
is never released. Your own `TestConcatMissingOffsetsDiagnostic` catches it
because of the `memory.NewCheckedAllocator` + `AssertSize(t, 0)` — running that
test against `main` reports `LEAK of 64 bytes FROM ...concatOffsets`. Nice that
the test covers the allocator state and not just the message; that's what made
the leak visible.
Might be worth a line in the commit message so it's findable later.
##########
arrow/array/concat.go:
##########
@@ -747,7 +768,7 @@ func concat(data []arrow.ArrayData, mem memory.Allocator)
(arr arrow.ArrayData,
}
case *arrow.ListType:
offsetWidth := dt.Layout().Buffers[1].ByteWidth
- offsetBuffer, valueRanges, err :=
concatOffsets(gatherFixedBuffers(data, 1, offsetWidth), offsetWidth, mem)
+ offsetBuffer, valueRanges, err := concatOffsetsData(data,
offsetWidth, out.length, mem)
if err != nil {
return nil, err
}
Review Comment:
Follow-up worth considering, either here or as a separate PR:
`*arrow.MapType` further down still calls
`concatOffsets(gatherFixedBuffers(data, 1, offsetWidth), offsetWidth, mem)` and
therefore still has the exact bug you just fixed for List.
I probed it on this branch:
```
map err = arrow/concat: runtime error: index out of range [0] with length 0
checked_allocator.go:191: LEAK of 64 bytes
```
I tried the one-line swap to `concatOffsetsData(data, offsetWidth,
out.length, mem)` and it works — the probe becomes `array/concat: array is
missing an offset buffer` with no leak, and `./arrow/array/...` plus
`./arrow/compute/...` pass clean.
That would also leave `concatOffsets`, `handle32BitOffsets`, and
`handle64BitOffsets` with no callers, so they could be deleted. Bonus: that
retires the `expand := src[:len(src)+1]` trick in `handle32BitOffsets`, which
reads one element past a slice's length and relies on the gathered buffer
having spare capacity — the direct `src[begin:end]` / `src[end]` form you've
moved to is bounds-checked against the buffer's real length instead.
Entirely your call whether to widen this PR's scope; the List/LargeList
change stands on its own.
--
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]
