Skip to content

fix(compute): release temporary arrays in binary view casts#898

Open
fallintoplace wants to merge 4 commits into
apache:mainfrom
fallintoplace:fix-cast-binary-temp-release
Open

fix(compute): release temporary arrays in binary view casts#898
fallintoplace wants to merge 4 commits into
apache:mainfrom
fallintoplace:fix-cast-binary-temp-release

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

CastBinaryToBinaryView and CastBinaryViewToBinary create a temporary result array, transfer its buffers with ArraySpan.TakeOwnership, and return without releasing the temporary array. TakeOwnership retains the buffers, so the temporary reference leaks on every cast.

What changes are included in this PR?

Release each temporary result after transferring its data to the output span. The output keeps its retained ownership, so successful cast behavior is unchanged.

Are these changes tested?

Yes. Both cast directions are exercised with a checked allocator bound to the compute context. Each test releases the cast output and verifies that memory returns to the pre-cast baseline while the input remains alive. The compute package passes normal and race tests.

Are there any user-facing changes?

Binary-to-binary-view and binary-view-to-binary casts no longer retain temporary allocations. There is no API change.

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 4, 2026 01:06

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zeroshade

Copy link
Copy Markdown
Member

@fallintoplace nudge — the fix itself is correct (TakeOwnership retains, so releasing the temporary is right). The two new tests just need scope.CheckSize(c.T()) called before in.Release() (or allocate in inside the scope) — as written they drop below the baseline and fail. Ping me for a re-review.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The production fix here (defer result.Release() after TakeOwnership) is correct — TakeOwnership retains, so dropping the builder's ref is right. The two new tests need rework though: they fail as written, and even after fixing the ordering they don't bind the cast to the checked allocator, so they'd pass regardless of the fix. Details plus a verified formulation inline. Requesting changes on the tests only.

Comment thread arrow/compute/cast_test.go Outdated
}

func (c *CastSuite) TestBinaryToBinaryViewReleasesTemporaryResult() {
scope := memory.NewCheckedAllocatorScope(c.mem)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two problems with this test:

  1. It fails as written — the scope is opened here, before in is allocated, but CheckSize runs while in is still alive, so it reports exp=0, got=192.
  2. More importantly, the cast runs on context.Background()DefaultAllocator, not c.mem, so the checked scope never sees the temporary. It passes with or without the production fix (I confirmed both), so it doesn't actually guard the leak.

To exercise the fix, allocate in first, open the scope after it, and bind the cast to the checked allocator:

in, _, err := array.FromJSON(c.mem, arrow.BinaryTypes.Binary, strings.NewReader(`["aGk=", "dGhpcyBpcyB0aGUgZmlyc3QgdGVzdCE=", null]`))
c.Require().NoError(err)

scope := memory.NewCheckedAllocatorScope(c.mem)
ctx := exec.WithAllocator(context.Background(), c.mem) // arrow/compute/exec

out, err := compute.CastArray(ctx, in, compute.SafeCastOptions(arrow.BinaryTypes.BinaryView))
c.Require().NoError(err)
out.Release()
scope.CheckSize(c.T())
in.Release()

I verified this fails before the fix (exp=192, got=33088) and passes after.

Comment thread arrow/compute/cast_test.go Outdated
}

func (c *CastSuite) TestBinaryViewToBinaryReleasesTemporaryResult() {
scope := memory.NewCheckedAllocatorScope(c.mem)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same two issues as the binary→view test above: it fails as written, and once the ordering is fixed it still doesn't bind the cast to c.mem, so it's vacuous. Apply the same pattern — build in, then open the scope, run the cast through exec.WithAllocator(context.Background(), c.mem), release out, CheckSize, then release in.

@fallintoplace fallintoplace changed the title fix: release temporary cast arrays after ownership transfer fix(compute): release temporary arrays in binary view casts Jul 17, 2026
@fallintoplace
fallintoplace force-pushed the fix-cast-binary-temp-release branch from f2f7d1c to 8651608 Compare July 17, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants