zeroshade commented on code in PR #1243:
URL: https://github.com/apache/arrow-go/pull/1243#discussion_r3884123154
##########
arrow/extensions/variant.go:
##########
@@ -225,6 +225,10 @@ func NewVariantType(storage arrow.DataType) (*VariantType,
error) {
dt = dt.(arrow.ExtensionType).StorageType()
}
+ if dt.ID() == arrow.NULL {
Review Comment:
This new rejection makes `NewShreddedVariantType(arrow.Null)` silently
return `nil`, because that constructor discards the error returned by
`NewVariantType`. Normal use of the returned type then panics. Please handle
`arrow.Null` with a valid non-nil representation or expose an error-returning
construction path instead of discarding the validation failure.
##########
arrow/extensions/variant.go:
##########
@@ -295,7 +299,10 @@ func validStruct(s *arrow.StructType) bool {
switch s.NumFields() {
case 1:
f := s.Field(0)
- return (f.Name == "value" && isBinary(f.Type)) || f.Name ==
"typed_value"
+ if f.Name == "value" {
+ return isBinary(f.Type)
+ }
+ return f.Name == "typed_value" && f.Type.ID() != arrow.NULL
Review Comment:
Checking only `f.Type.ID()` lets an extension whose storage type is
`arrow.Null` bypass this validation because its ID is `EXTENSION`. The same
bypass exists in the two-field path below. Please unwrap extension storage
before checking for Null in both nested `typed_value` forms and add one- and
two-field regression tests.
--
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]
