zeroshade commented on code in PR #898:
URL: https://github.com/apache/arrow-go/pull/898#discussion_r3554173684
##########
arrow/compute/cast_test.go:
##########
@@ -1668,6 +1668,30 @@ func (c *CastSuite) TestBinaryViewToBinaryLike() {
})
}
+func (c *CastSuite) TestBinaryToBinaryViewReleasesTemporaryResult() {
+ scope := memory.NewCheckedAllocatorScope(c.mem)
Review Comment:
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:
```go
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.
##########
arrow/compute/cast_test.go:
##########
@@ -1668,6 +1668,30 @@ func (c *CastSuite) TestBinaryViewToBinaryLike() {
})
}
+func (c *CastSuite) TestBinaryToBinaryViewReleasesTemporaryResult() {
+ scope := memory.NewCheckedAllocatorScope(c.mem)
+
+ in, _, err := array.FromJSON(c.mem, arrow.BinaryTypes.Binary,
strings.NewReader(`["aGk=", "dGhpcyBpcyB0aGUgZmlyc3QgdGVzdCE=", null]`))
+ c.Require().NoError(err)
+
+ out, err := compute.CastArray(context.Background(), in,
compute.SafeCastOptions(arrow.BinaryTypes.BinaryView))
+ c.Require().NoError(err)
+ out.Release()
+ scope.CheckSize(c.T())
+ in.Release()
+}
+
+func (c *CastSuite) TestBinaryViewToBinaryReleasesTemporaryResult() {
+ scope := memory.NewCheckedAllocatorScope(c.mem)
Review Comment:
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`.
--
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]
