serramatutu commented on code in PR #833:
URL: https://github.com/apache/arrow-go/pull/833#discussion_r3847193880
##########
arrow/array/struct_test.go:
##########
@@ -472,52 +474,48 @@ func TestStructArrayUnmarshalJSONMissingFields(t
*testing.T) {
name string
jsonInput string
want string
- panic bool
+ panicErr error
}{
{
name: "missing required field",
jsonInput: `[{"f2": 3, "f3": {"f3_1": "test"}}]`,
- panic: true,
+ panicErr: errors.New("arrow/array: index out of
range"),
want: "",
},
{
name: "missing optional fields",
jsonInput: `[{"f2": 3, "f3": {"f3_3": "test"}}]`,
- panic: false,
+ panicErr: nil,
want: `{[(null)] [3] {[(null)] [(null)]
["test"]}}`,
},
+ {
+ name: "explicit null in required field",
+ jsonInput: `[{"f2": 3, "f3": {"f3_3": null}}]`,
+ panicErr: errors.New("field 'f3_3' is non-nullable but
got null"),
+ want: "",
+ },
}
for _, tc := range tests {
t.Run(
tc.name, func(t *testing.T) {
-
- var val bool
-
sb := array.NewStructBuilder(pool, dtype)
defer sb.Release()
- if tc.panic {
- defer func() {
- e := recover()
- 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 {
- t.Fatalf("invalid
error. got=%q, want=%q", got, want)
- }
- }()
- } else {
- defer func() {
- if e := recover(); e != nil {
- t.Fatalf("unexpected
panic: %v", e)
- }
- }()
- }
+ defer func() {
+ e := recover()
+ if e == nil && tc.panicErr != nil {
+ t.Fatalf("did not panic,
expected panic: %v", tc.panicErr)
+ } else if e != nil && tc.panicErr ==
nil {
+ t.Fatalf("unexpected panic:
%v", e)
+ } else if e != nil && tc.panicErr !=
nil && fmt.Errorf("%s", e).Error() != tc.panicErr.Error() {
+ t.Fatalf("invalid error.
got=%v, want=%v", e, tc.panicErr.Error())
+ }
+ }()
err := sb.UnmarshalJSON([]byte(tc.jsonInput))
if err != nil {
- t.Fatal(err)
+ panic(err)
}
Review Comment:
Fixed. The test no longer converts errors into panics: it asserts the
returned error with `require.ErrorContains` (and `main` has since rewritten
this test to drop the panic-recover harness altogether).
##########
arrow/array/record.go:
##########
@@ -434,43 +433,69 @@ func (b *RecordBuilder) UnmarshalOne(dec *json.Decoder)
error {
return fmt.Errorf("record should start with '{', not %s", t)
}
- keylist := make(map[string]bool)
+ // consume one row checking for duplicates and nulls
+ keylist := make(map[string]json.RawMessage)
for dec.More() {
keyTok, err := dec.Token()
if err != nil {
return err
}
key := keyTok.(string)
- if keylist[key] {
+ if _, ok := keylist[key]; ok {
return fmt.Errorf("key %s shows up twice in row to be
decoded", key)
}
- keylist[key] = true
+
+ var val json.RawMessage
+ if err := dec.Decode(&val); err != nil {
+ return err
+ }
indices := b.schema.FieldIndices(key)
if len(indices) == 0 {
- var extra interface{}
- if err := dec.Decode(&extra); err != nil {
- return err
- }
continue
}
- if err := b.fields[indices[0]].UnmarshalOne(dec); err != nil {
- return err
+ idx := indices[0]
+
+ if bytes.Equal(val, []byte("null")) &&
!b.schema.Field(idx).Nullable {
+ return fmt.Errorf("field '%s' is non-nullable but got
null", key)
}
+
+ keylist[key] = val
}
// consume the closing '}'
if _, err := dec.Token(); err != nil {
return err
}
+ // check that all non-nullable fields were specified
+ for i := 0; i < b.schema.NumFields(); i++ {
+ f := b.schema.Field(i)
+ if _, ok := keylist[f.Name]; !ok && !f.Nullable {
+ return fmt.Errorf("field '%s' is required but no value
was given", f.Name)
+ }
+ }
+
+ // at this point we know there are no integrity errors, append values
to field builders
+ for key, val := range keylist {
+ valDec := json.NewDecoder(bytes.NewReader(val))
+ valDec.UseNumber()
+
+ indices := b.schema.FieldIndices(key)
+ if err := b.fields[indices[0]].UnmarshalOne(valDec); err != nil
{
+ return err
+ }
+ }
Review Comment:
Fixed. `unmarshalOne` validates the whole buffered row up front, and any
error from a field builder is rolled back through the per-row
`builderCheckpoint` added in #1113, so column lengths can no longer diverge.
--
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]
