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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
