docs(compute): attach the NewDatumWithoutOwning doc comment and say what Release does to it - #1320
singhpratech wants to merge 1 commit into
Conversation
…hat Release does to it
The comment above NewDatumWithoutOwning was separated from the declaration
by a blank line, so go doc and pkg.go.dev show the function with no
documentation at all. The text itself had a broken sentence and ended in
stray characters ("convenience function.+-").
The comment now sits on the declaration and states the contract: the
Datum owns nothing, the caller keeps the value alive and must not call
Release on the Datum, and a Release call releases the caller's reference,
which with a C-backed allocator or cdata-imported buffers frees memory
under a live value.
Documentation only. Whether a non-owning Datum should refuse Release is
left to apache#1298.
…ing doc comment for #1298
zeroshade
left a comment
There was a problem hiding this comment.
The proposed ownership rule is not correct for every accepted call pattern. NewDatumWithoutOwning does not add a reference, but that does not imply callers must never release the returned Datum.
arrow/compute/exprs/exec.go currently returns compute.NewDatumWithoutOwning(scalar.NewMapScalar(kvArr)). That releasable scalar is created inline, so the Datum is its only surviving handle; not releasing it leaks the scalar's retained Arrow data. ScalarDatum.Release delegates to the wrapped scalar's Release, and downstream expression evaluation currently releases these returned Datums.
Please describe the actual contract: no reference is added, and Datum.Release consumes the supplied reference. Borrowed inputs must remain owned and alive elsewhere; ownership-transferred temporaries must eventually be released through the Datum. Alternatively, forbid temporaries and update the existing caller.
…ract is a consumed reference, not a forbidden release
Rationale for this change
The comment above
NewDatumWithoutOwningis separated from the declaration by a blank line, sogo docand pkg.go.dev show the function with no documentation at all. The text itself has asentence with no main clause and ends in stray characters (
convenience function.+-). Thefunction's one hazard, a
Releasecall that releases the caller's reference (#1298), is the thinga reader most needs to see and currently cannot.
What changes are included in this PR?
The comment now sits on the declaration and states the contract: the returned
Datumowns nothing,the caller keeps the value alive for as long as the
Datumis in use and must not callReleaseonit, and since the
Datumis an ordinaryArrayDatum/ChunkedDatum/RecordDatum/TableDatum/ScalarDatum, aReleasecall compiles and releases the caller's reference, which with a C-backedallocator or cdata-imported buffers frees memory under a live value. It ends by pointing at
NewDatumfor the owning case.Are these changes tested?
Documentation only.
go doc ./arrow/compute NewDatumWithoutOwningrenders the two paragraphs;gofmt and
go vet ./arrow/compute/are clean.Are there any user-facing changes?
Documentation only. Whether a non-owning
Datumshould refuseRelease(a no-op, a distincttype, or a panic) is a separate decision and stays with #1298; this change does not close it.