Asthenia0412 commented on PR #1377:
URL: https://github.com/apache/arrow-go/pull/1377#issuecomment-6296137846
@zeroshade — thanks again for the second pass. Quick recap of what's
addressed and confirmed:
**1. `!` in title/body — NOT a breaking change** ✅
Title is `fix(arrow/compute): fix NewDatumWithoutOwning ownership
semantics` (no `!`). The godoc uses a neutral note: "Release on a Datum from
NewDatumWithoutOwning is a no-op, so use NewDatum when the Datum should own the
value."
**2. Regression test depth (datum_test.go)** ✅
I validated the test structure for all 5 Datum kinds (Array, Chunked,
RecordBatch, Table, Scalar/list):
- Each case sets up a single-refcount caller-owned value
- Calls NewDatumWithoutOwning(v).Release() (should be no-op)
- Asserts mem.CurrentAlloc() unchanged
- Caller's final Release() brings allocations back to 0 via defer
mem.AssertSize(t, 0)
Experimentally verified that disabling borrowed=true in any case causes
CurrentAlloc() to drop from 128 to 0, so the assertions WOULD fail — the test
catches the bug for all variants. Earlier test version you reviewed may have
predated the full rewrite.
**3. Executor.go out-of-scope runtime.NumCPU() change — reverted**
Discovered my branch accidentally carried a runtime.NumCPU() vs
runtime.GOMAXPROCS(0) diff that is now superseded by apache/arrow-go#1384.
Pushed a commit dropping it so the diff is purely datum-ownership semantics.
**Build**: go build ./arrow/compute/... and go vet ./arrow/compute/ both
pass. Waiting on Actions CI.
--
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]
