zeroshade commented on code in PR #1157:
URL: https://github.com/apache/arrow-go/pull/1157#discussion_r3856599345
##########
arrow/array/union_test.go:
##########
@@ -78,6 +78,45 @@ func TestUnionBuilderChildBounds(t *testing.T) {
}
}
+func TestUnionBuilderRejectsInvalidJSONTypeCodes(t *testing.T) {
+ fields := []arrow.Field{{Name: "value", Type:
arrow.PrimitiveTypes.Int32}}
+ cases := []struct {
+ name string
+ new func() array.UnionBuilder
+ }{
+ {
+ name: "dense",
+ new: func() array.UnionBuilder {
+ return
array.NewDenseUnionBuilder(memory.DefaultAllocator, arrow.DenseUnionOf(fields,
[]arrow.UnionTypeCode{0}))
+ },
+ },
+ {
+ name: "sparse",
+ new: func() array.UnionBuilder {
+ return
array.NewSparseUnionBuilder(memory.DefaultAllocator,
arrow.SparseUnionOf(fields, []arrow.UnionTypeCode{0}))
+ },
+ },
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ for _, typeCode := range []string{"256", "-1", "127",
"1.5", "null"} {
+ builder := tc.new()
+ err := func() (err error) {
+ defer func() {
+ if r := recover(); r != nil {
Review Comment:
**Blocking:** This recovery wrapper converts the panic being guarded against
into an `error`, after which `assert.Error` passes. I verified that deleting
both new bounds checks still leaves this test green: the `127` cases panic
during indexing, are converted here, and satisfy the assertion. Please let
unexpected panics fail naturally—or assert no panic separately—then assert that
decoding returned an error.
##########
arrow/array/union.go:
##########
@@ -776,6 +776,45 @@ type unionBuilder struct {
typesBuilder *int8BufferBuilder
}
+func unionTypeCodeFromJSON(dec *json.Decoder, typeID any, typ arrow.DataType)
(arrow.UnionTypeCode, error) {
+ var id int64
+ switch tid := typeID.(type) {
+ case json.Number:
+ var err error
+ id, err = tid.Int64()
+ if err != nil {
+ return 0, err
+ }
+ case float64:
Review Comment:
**Blocking:** Default-decoder entry points validate the already-rounded
`float64`, so invalid numeric type codes can become valid before this check.
For both dense and sparse builders, `AppendValueFromString("[-1e-400, 1]")`
accepts the mathematically negative value as code `0`, and
`[127.00000000000000001, 1]` is accepted as code `127`. `UnmarshalJSON` rejects
the same inputs because it enables `UseNumber`, so the public entry points are
inconsistent. Please preserve the numeric lexeme before validation across
`AppendValueFromString`/`UnmarshalOne` as well, and add regression coverage for
underflow and rounded fractional values.
--
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]
