Re: [PR] fix(arrow/compute): fix NewDatumWithoutOwning ownership semantics [arrow-go]

["Asthenia0412 (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org