zeroshade commented on code in PR #1485:
URL: https://github.com/apache/arrow-go/pull/1485#discussion_r4515303496
##########
arrow/compute/exprs/exec.go:
##########
@@ -117,11 +118,11 @@ func makeExecBatch(ctx context.Context, schema
*arrow.Schema, partial compute.Da
// type. for now we'll cast this
val, err = val.CastTo(field.Type)
if err != nil {
- return compute.ExecBatch{}, err
+ return out, err
Review Comment:
Before #1474, `scalar.Struct` had no `Retain`, so `out.Values[i] =
compute.NewDatum(val)` a few lines below takes no reference to a nested struct
field. Now that this error path returns `out` and the deferred cleanup releases
it, a later failing cast releases the caller's nested data.
I reproduced it on this branch: the input was a struct scalar `{s: {x: [1,
2, 3]}, b: "notanint"}` and the target schema `{s: struct<x: list<int32>>, b:
int32}`. After `makeExecBatch` fails on the cast of `b`, the caller's `s.x`
list has lost its data, and reading it panics with a nil pointer dereference.
With main merged in (#1474's `Struct.Retain`), the list stays intact and the
checked allocator ends at 0.
Before this PR the same path leaked instead, so this change is only safe
together with #1474. That's why the rebase and a regression test for this case
would be good.
--
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]
