hcrosse opened a new pull request, #1485: URL: https://github.com/apache/arrow-go/pull/1485
### Rationale for this change A failed `ExecuteScalarExpression` keeps the input columns it already took, and releasing a null dictionary scalar created by the package leaves its empty dictionary referenced. ### What changes are included in this PR? New null scalars now go into an owning `ScalarDatum` instead of `NewDatumWithoutOwning`, whose `Release` does nothing. `NewDatum` would retain them a second time. This covers missing fields and null casts in `makeExecBatch`, Substrait null literals and `compute.NullLiteral`. `TestIsNullDictionaryScalar`, which relied on the old behavior, now builds an owning datum too. On error, `makeExecBatch` now returns its partial batch so the deferred cleanup can release the values, then the cleanup resets the result. Callers still get an empty batch and the same error. The reset is required for a struct array input, which `makeExecBatch` converts to a record and handles recursively. Without it, the outer cleanup releases values that the recursive call already released. This PR doesn't change the extra reference that `makeExecBatch` takes on a successful cast result. Releasing it needs the releasable struct and run-end encoded scalars in https://github.com/apache/arrow-go/pull/1474. ### Are these changes tested? Yes. I added four tests: - `TestMakeExecBatchReleasesNullScalars` covers missing record fields, missing struct scalar fields and null casts to a dictionary type. - `TestLiteralToDatumNullUserDefinedRelease` covers a Substrait null literal whose user defined type is a dictionary. - `TestNullLiteralRelease` covers `compute.NullLiteral` of a dictionary type. - `TestMakeExecBatchErrorReleases` covers failed casts from record, struct array and struct scalar inputs, and ambiguous record fields. The null scalar tests check that releasing the datum clears the dictionary's buffers. The error tests use a checked allocator and require an empty batch. All cases fail on main and pass with this change. The issue's reproduction prints 0 for both counts. On macOS arm64 with Go 1.26.10, `go test ./arrow/...` passed with arrow-testing and parquet-testing checked out. `go test -race -tags assert,test` on `arrow/compute` and `arrow/compute/exprs`, `go vet ./arrow/compute/...`, `golangci-lint` 2.13.2 in `arrow`, `parquet` and `internal`, and the Release Audit Tool also passed. ### Are there any user-facing changes? Yes. Releasing these null literals and batches now releases their scalars, and a failed `ExecuteScalarExpression` releases the columns it took. There is no API change. Closes https://github.com/apache/arrow-go/issues/1481. -- 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]
