zeroshade commented on PR #1021: URL: https://github.com/apache/arrow-go/pull/1021#issuecomment-5096465323
Re-reviewed — the head is still `b9ef731d` with no new commits, so my earlier assessment stands unchanged and I am going to keep this closed to merging for now. To restate the blocker concisely: the check added in `newData()` enforces something that is not actually an invariant of `StructBuilder`. Populating a struct purely through `FieldBuilder(i)` without calling `StructBuilder.Append()` leaves `b.length` at 0, and that is a supported pattern — `MapScalar` in `arrow/scalar` does exactly this, so with this patch applied `arrow/scalar` `TestMapScalarBasics` panics with `field_length=2, struct_length=0`. I confirmed the attribution by running the whole module: #1018, #1019, #1020, #1023 and #1026 applied together introduce zero new package-level failures, while this PR uniquely breaks `arrow/scalar`. If you would like to pursue the underlying observation — which is legitimate, a short field really does yield a bad array and a long one silently drops values — the two viable routes are: 1. Add the check to `Struct`'s `ValidateFull()` instead of `newData()`, matching how `arrow/array` already handles structural validation opt-in (see `fullyValidateOffsetsAndSizes` for list types). This is the path I would take and I would review it promptly. 2. If you believe the builder contract itself should change, that needs a discussion on [email protected] first, since it turns working user code into a panic. Happy to look again if you take route 1. -- 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]
