fix(compute/exprs): avoid empty field reference panic - #1130
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
Correct and minimal.
out starts nil and is only assigned inside the StructFieldRef case, so on the first iteration with an empty field list the old code dereferenced a nil *arrow.Field to read out.Type — a panic in a function that otherwise returns errors. The guard is in the right spot.
Worth noting what this deliberately doesn't do: it would have been easy to hoist the emptiness check above the loop, but keeping it inside preserves the distinction between two different conditions — an empty schema at the top level, where there's no type to name, versus a nested reference into a field whose type has no children, where ErrNoChildren: <type> is genuinely useful. The nested path still reaches the original line with out non-nil, so that detail survives.
Both entry points are covered too, since GetRefFieldFromSchema funnels through here with schema.Fields() — an empty schema takes the same path, so the panic was reachable from the public API as well.
Note: this review was drafted with AI assistance by a maintainer and may contain mistakes. If anything here looks wrong, say so on the PR and I'll take another look.
| } | ||
| } | ||
|
|
||
| func TestGetRefFieldEmptySchema(t *testing.T) { |
There was a problem hiding this comment.
Minor coverage gap: this pins the top-level empty case, but nothing pins the nested one. A future refactor that hoists the len(fields) == 0 check above the loop would silently drop the : <type> detail from nested references and this test would still pass.
A second case — a reference into a field whose type has no children, asserting the error still names the type — would guard the behaviour the fix went out of its way to preserve.
Rationale for this change
A field reference against an empty schema tries to include out.Type in the error before out has been set. This causes a nil pointer panic instead of returning ErrNoChildren.
What changes are included in this PR?
Return ErrNoChildren for an empty schema before formatting a field type. Preserve the existing error detail for nested references and add regression coverage.
Are these changes tested?
go test ./arrow/compute/exprsAre there any user-facing changes?
Invalid field references now return an error instead of panicking when the schema has no fields.