Asthenia0412 commented on PR #1377: URL: https://github.com/apache/arrow-go/pull/1377#issuecomment-6272575915
@zeroshade thanks again for the thorough review — pushed an update that addresses every actionable point: - **Breaking-change surface (datum.go:147, datum.go:307)**: PR title updated to `fix(arrow/compute)!:` and the godoc for `NewDatumWithoutOwning` now calls out the transfer-pattern leak (Release() is now a no-op on non-owning Datums, callers that moved ownership via the old transfer pattern must switch to `NewDatum` or keep a reference themselves). - **Example (datum.go:321)**: replaced the fake `NewInt32Array` with a compilable `array.NewInt32Builder(memory.DefaultAllocator)` snippet. - **executor.go:1289**: added a `default:` branch that wraps `arrow.ErrInvalid` with the concrete type from `Take()`, so any future aggregate result variant surfaces as an error instead of nil-out + panic at `Release()`. - **Regression test (`arrow/compute/datum_test.go`)**: `TestNewDatumWithoutOwningDoesNotLeakOwner` uses a `CheckedAllocator` to assert `CurrentAlloc()` is unchanged after `NewDatumWithoutOwning(v).Release()` for every Datum kind (Array, Chunked, RecordBatch, Table, Scalar) — i.e. the original 128-byte leak from #1298 cannot regress. On the one semantic nit I did **not** change: the `nil → EmptyDatum` vs old `nil → scalar.MakeScalar(nil)` path in executor.go. `AggregateResult.Take()` currently only produces `nil`, `scalar.Scalar`, or `arrow.ArrayData`, and the new `default:` branch guards future extensions, so I kept `EmptyDatum` as the canonical no-value result. Let me know if you'd rather preserve the old null-scalar semantic and I'll adjust. Locally: `go build ./arrow/compute/...`, `go vet ./arrow/compute/`, and the new datum tests all pass. Waiting on Actions for the full CI sweep. -- 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]
