zeroshade commented on code in PR #1021:
URL: https://github.com/apache/arrow-go/pull/1021#discussion_r3660288919
##########
arrow/array/struct.go:
##########
@@ -439,6 +439,12 @@ func (b *StructBuilder) NewStructArray() (a *Struct) {
}
func (b *StructBuilder) newData() (data *Data) {
+ for i, f := range b.fields {
+ if f.Len() != b.length {
+ panic(fmt.Errorf("%w: arrow/array: struct field builder
length must equal struct length (field=%d, field_length=%d, struct_length=%d)",
+ arrow.ErrInvalid, i, f.Len(), b.length))
+ }
+ }
Review Comment:
This is the breaking check. `b.length` only advances when
`StructBuilder.Append()` / `AppendValues()` is called, but it is entirely
legitimate to populate a struct purely through `FieldBuilder(i)` and leave
`b.length` at 0 — `MapScalar` in `arrow/scalar` does precisely this, and it
panics here.
Also note this panics rather than returning an error, so it converts a
previously-working call into a crash for downstream users. Even if the
invariant were correct, `newData()` is the wrong place given `arrow/array`'s
opt-in validation design (`fullyValidateOffsetsAndSizes` and friends live
behind `ValidateFull`).
##########
arrow/array/struct_test.go:
##########
@@ -646,7 +666,7 @@ func TestStructArrayUnmarshalJSONMissingFields(t
*testing.T) {
if e == nil {
t.Fatalf("this should
have panicked, but did not; slice value %v", val)
}
- if got, want := e.(string),
"arrow/array: index out of range"; got != want {
+ if got, want := fmt.Sprint(e),
"invalid: arrow/array: struct field builder length must equal struct length
(field=2, field_length=0, struct_length=1)"; got != want {
Review Comment:
Changing an existing test's expected failure is worth calling out
explicitly. On `main` this case panics with the string `arrow/array: index out
of range` at **read** time (from `arr.String()` touching a short field), not at
build time. This PR makes it fail earlier, inside `NewStructArray()`, with a
different type (an `error` rather than a `string`), which is why the assertion
had to be rewritten to `fmt.Sprint(e)`.
Failing fast is defensible in isolation, but combined with the
`arrow/scalar` regression it indicates the check is too broad rather than that
the old expectation was wrong.
--
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]
