fix: release temporary cast arrays after ownership transfer#898
fix: release temporary cast arrays after ownership transfer#898fallintoplace wants to merge 3 commits into
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The production fix is correct and worth having — ArraySpan.TakeOwnership retains the buffers (arrow/compute/exec/span.go), so the ba.bldr.NewArray() temporary was leaking without the defer result.Release().
But both new tests fail:
cast_test.go:1682: invalid memory size exp=192, got=0
cast_test.go:1694: invalid memory size exp=32896, got=0
--- FAIL: TestCasts/TestBinaryToBinaryViewReleasesTemporaryResult
--- FAIL: TestCasts/TestBinaryViewToBinaryReleasesTemporaryResult
The scope is created after in is allocated, but the test releases in inside the scope before CheckSize, so outstanding drops below the baseline (exp = size of in, got = 0). Either call scope.CheckSize(c.T()) before in.Release(), or allocate in inside the scope. Once the harness is fixed these will lock the fix in nicely. Verified against the PR head with go test.
|
@fallintoplace nudge — the fix itself is correct ( |
Summary
Problem
CastBinaryToBinaryViewandCastBinaryViewToBinaryeach create a temporary result array withba.bldr.NewArray(), transfer ownership of its buffers without.TakeOwnership(result.Data()), and then return without releasing the temporary array.TakeOwnershiponly transfers buffer ownership, so the temporary array retains its own reference and can leak memory per cast.Change
defer result.Release()after creating the temporary result array in:CastBinaryToBinaryViewCastBinaryViewToBinaryTakeOwnershipbehavior intact.Impact
Testing
arrow/compute/cast_test.go:TestBinaryToBinaryViewReleasesTemporaryResultTestBinaryViewToBinaryReleasesTemporaryResultmemory.NewCheckedAllocatorScopeafter releasing cast outputs and inputs.