zeroshade commented on code in PR #1480:
URL: https://github.com/apache/arrow-go/pull/1480#discussion_r4515131614
##########
arrow/extensions/variant.go:
##########
@@ -347,14 +352,27 @@ func isNullType(dt arrow.DataType) bool {
return st == nil || st.ID() == arrow.NULL
}
+func isUnsignedInteger(dt arrow.DataType) bool {
+ st := storageType(dt)
+ if st == nil {
+ return false
+ }
+ switch st.ID() {
+ case arrow.UINT8, arrow.UINT16, arrow.UINT32, arrow.UINT64:
+ return true
+ default:
+ return false
+ }
+}
+
func validStruct(s *arrow.StructType) bool {
switch s.NumFields() {
case 1:
f := s.Field(0)
if f.Name == "value" {
return isBinary(f.Type)
}
- return f.Name == "typed_value" && !isNullType(f.Type)
+ return f.Name == "typed_value" && !isNullType(f.Type) &&
!isUnsignedInteger(f.Type)
Review Comment:
This case doesn't recurse into a nested `typed_value`, so an unsigned type
deeper down still gets through. For example, this is accepted (`NewVariantType`
returns no error):
```go
typed_value: list<struct{typed_value: struct{n: struct{typed_value:
uint32}}}>
```
The missing recursion predates this PR, but it means "including nested
shredded fields" only holds when the struct also has a `value` field. Could
this call `validNestedType` when `f.Type` is nested, like the two-field case
below, and add a test case for it?
##########
arrow/extensions/variant.go:
##########
@@ -148,8 +148,10 @@ func NewShreddedVariantType(dt arrow.DataType)
*VariantType {
// 3. Must satisfy exactly one of the following:
// a. MUST have non-nullable field named "value" that is
binary/largebinary/binary_view
// b. MUST have an nullable field named "value" that is
binary/largebinary/binary_view
-// and another nullable field named "typed_value" that is either a
primitive type or
-// a list/large_list/list_view or struct which also satisfies the
following requirements:
+// and another nullable field named "typed_value" that is either a
primitive type
+// (other than an unsigned integer) or a list/large_list/list_view or
struct which
+// also satisfies the following requirements. Unsigned integers are not
valid
Review Comment:
nit: "(other than an unsigned integer)" two lines up already says this, so
this sentence can go.
##########
arrow/extensions/variant_test.go:
##########
@@ -1199,23 +1237,23 @@ func TestManyTypesShredded(t *testing.T) {
)},
arrow.Field{Name: "uint8", Type: arrow.StructOf(
Review Comment:
nit: the field names `uint8`/`uint16`/`uint32` no longer match their
`Int16`/`Int32`/`Int64` typed_value types. Could you rename them (e.g.
`int16_from_uint8`) or add a comment that unsigned values are shredded into the
next wider signed type?
--
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]
