hcrosse opened a new issue, #1481:
URL: https://github.com/apache/arrow-go/issues/1481
### Describe the bug, including details regarding any error messages,
version, and platform.
Two paths in `arrow/compute` keep buffers referenced after the caller
releases everything. When `ExecuteScalarExpression` fails to prepare its input,
columns it already took stay referenced. Releasing a null dictionary scalar
that the package created, such as `compute.NullLiteral` of a dictionary type,
leaves its empty dictionary referenced.
I reproduced both on main at `66722e30` on macOS arm64 with Go 1.26.10. With
a synthetic record of one row, a failed cast of column `b` leaves 192 bytes
allocated for column `a` after the input is released. Releasing
`compute.NullLiteral` for `dictionary<int32, utf8>` leaves another 64 bytes.
With v18.8.0 only the first leak reproduces.
**Reproduction**
Save this as `main.go` in a new subdirectory of an arrow-go checkout at
`66722e30df98ddd6f7bb46023fa13287b84308f6`, then run `go run .` from that
directory:
```go
package main
import (
"context"
"fmt"
"strings"
"github.com/apache/arrow-go/v18/arrow"
"github.com/apache/arrow-go/v18/arrow/array"
"github.com/apache/arrow-go/v18/arrow/compute"
"github.com/apache/arrow-go/v18/arrow/compute/exprs"
"github.com/apache/arrow-go/v18/arrow/memory"
)
func main() {
// 1. A failed cast leaks the column added before it.
mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
schema := arrow.NewSchema([]arrow.Field{
{Name: "a", Type: arrow.BinaryTypes.Binary, Nullable: true},
{Name: "b", Type: arrow.PrimitiveTypes.Int32, Nullable: true},
}, nil)
rec, _, _ := array.RecordFromJSON(mem, arrow.NewSchema([]arrow.Field{
{Name: "a", Type: arrow.BinaryTypes.Binary, Nullable: true},
{Name: "b", Type: arrow.BinaryTypes.String, Nullable: true},
}, nil), strings.NewReader(`[{"a": "eA==", "b": "x"}]`))
bldr := exprs.NewExprBuilder(exprs.NewDefaultExtensionSet())
_ = bldr.SetInputSchema(schema)
ex, _ := bldr.FieldRef("a").BuildExpr()
_, err :=
exprs.ExecuteScalarExpression(compute.WithAllocator(context.Background(), mem),
schema, ex, compute.NewDatumWithoutOwning(rec))
rec.Release()
fmt.Printf("cast error: %v\nbytes still allocated: %d\n", err,
mem.CurrentAlloc())
// 2. A null dictionary literal never releases its empty dictionary.
dflt := memory.NewCheckedAllocator(memory.NewGoAllocator())
memory.DefaultAllocator = dflt
compute.NullLiteral(&arrow.DictionaryType{IndexType:
arrow.PrimitiveTypes.Int32, ValueType: arrow.BinaryTypes.String}).Release()
fmt.Printf("NullLiteral(dictionary) bytes still allocated: %d\n",
dflt.CurrentAlloc())
}
```
On main:
```text
cast error: invalid: strconv.ParseInt: parsing "x": invalid syntax
bytes still allocated: 192
NullLiteral(dictionary) bytes still allocated: 64
```
**Expected behavior**
The failed cast should return the same error, and both counts should be zero.
**Cause**
When a cast fails or a field name is ambiguous, `makeExecBatch` returns a
new empty `ExecBatch`. That replaces its named result before the deferred
cleanup reads it, so the values already in the batch are never released:
https://github.com/apache/arrow-go/blob/66722e30df98ddd6f7bb46023fa13287b84308f6/arrow/compute/exprs/exec.go#L40-L50
https://github.com/apache/arrow-go/blob/66722e30df98ddd6f7bb46023fa13287b84308f6/arrow/compute/exprs/exec.go#L66-L72
https://github.com/apache/arrow-go/pull/1464 gave null dictionary scalars an
empty dictionary and wrapped the new scalars in `NewDatumWithoutOwning`.
https://github.com/apache/arrow-go/pull/1377 landed just before it and made
`NewDatumWithoutOwning` borrow its value, with a `Release` that does nothing.
Since both landed, releasing those datums no longer releases the scalars. The
leak measurement in https://github.com/apache/arrow-go/pull/1464 was taken
before https://github.com/apache/arrow-go/pull/1377 landed.
https://github.com/apache/arrow-go/blob/66722e30df98ddd6f7bb46023fa13287b84308f6/arrow/compute/expression.go#L602-L604
The same wrapping is used for missing fields and null casts in
`makeExecBatch`, and for Substrait null literals, which can have a dictionary
type through a registered user defined type. The empty dictionary comes from
`memory.DefaultAllocator`. The Go allocator reclaims it through garbage
collection, so the leak shows with checked or C allocators. The failed cast
leak holds the caller's own buffers whatever their allocator.
I have a patch with tests and can open a PR.
### Component(s)
Other
--
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]
