hcrosse opened a new pull request, #1470:
URL: https://github.com/apache/arrow-go/pull/1470

   ### Rationale for this change
   
   A nested struct field reference returns a value borrowed from the input 
without retaining it. Releasing the result can free buffers the input still 
uses. With a struct scalar input, the result can be freed before 
`ExecuteScalarExpression` returns.
   
   On main, the issue's `equal(s.a, 1)` filter panics with `too many releases` 
under `-tags assert,test` when the record is released. Without assertions, 
reading a binary field from a struct scalar input silently returns an empty 
value.
   
   ### What changes are included in this PR?
   
   `execFieldRef` takes a reference after the type check when the remaining 
path contains only struct fields. This extends the retain that 
https://github.com/apache/arrow-go/pull/1466 added for top-level columns, while 
keeping its type-mismatch behavior. On scalar inputs, list-element and 
map-value references return new scalars that the caller already owns, so they 
are not retained again. `GetReferencedValue` is unchanged.
   
   This change leaves three related problems alone:
   
   - A struct-typed result from a scalar input is still released more times 
than it is retained and can be freed before return, because `scalar.Struct` has 
`Release` but no `Retain`.
   - Array-input list elements are returned already released by 
`GetReferencedValue`.
   - A type mismatch on a list or map path leaves the newly created scalar 
unreleased.
   
   ### Are these changes tested?
   
   Yes. I added `TestNestedFieldRefResultOwnership` with three subtests, all 
using a checked allocator:
   
   - `array struct field` reads `s.a`, releases the record and checks that the 
result still holds its buffers and values. It fails on main because the buffers 
have been freed.
   - `scalar struct field` reads the binary field `s.b` from a struct scalar 
input and checks that the result still holds `hello` after the input is 
released. It fails on main. A separate probe showed that on main the buffer is 
already freed when `ExecuteScalarExpression` returns.
   - `scalar list element` reads `s.l[1]` and checks that it contains `world` 
without leaking. It passes on main, and it fails if the new list-element scalar 
is retained again.
   
   All three pass with this fix, with and without `-tags assert,test`. I also 
ran the issue's filter with the fix, and it passes without leaks.
   
   On macOS arm64 with Go `1.26.10`, `go test ./arrow/compute/exprs/...` 
passed, along with `go test -race -tags assert,test ./arrow/compute/exprs/`, 
`go vet`, `gofmt`, `golangci-lint` `2.13.2` in `arrow` and the Release Audit 
Tool.
   
   ### Are there any user-facing changes?
   
   Yes. Nested struct field references that return arrays, or scalars with 
buffers such as string, binary and list scalars, keep their buffers until the 
result is released. Filters such as `equal(s.a, 1)` no longer release a child 
the input still uses. There is no API change.
   
   Closes https://github.com/apache/arrow-go/issues/1451.
   


-- 
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]

Reply via email to