serramatutu commented on code in PR #833:
URL: https://github.com/apache/arrow-go/pull/833#discussion_r3847192619
##########
arrow/array/record.go:
##########
@@ -434,49 +436,74 @@ 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++ {
- if !keylist[b.schema.Field(i).Name] {
+ 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, so append
values to the
+ // field builders in schema order.
+ for i := 0; i < b.schema.NumFields(); i++ {
+ val, ok := keylist[b.schema.Field(i).Name]
+ if !ok {
b.fields[i].AppendNull()
+ continue
+ }
+
+ valDec := json.NewDecoder(bytes.NewReader(val))
+ valDec.UseNumber()
+ if err := b.fields[i].UnmarshalOne(valDec); err != nil {
+ b.Resize(-1)
Review Comment:
Fixed. `Resize(-1)` is gone from `RecordBuilder` — #1113 landed while this
PR was open and gives exactly the pre-row checkpoint you asked for, so
`UnmarshalOne` now relies on upstream's `builderCheckpoint` capture/restore.
Nullability is also validated before anything is appended now, so the
`{"a":1,"b":[2,"bad"]}` case is covered by upstream's rollback tests plus
`TestRecordBuilderRejectsNullInNestedNonNullableField` and
`TestRecordBuilderKeepsDecodingAfterRejectedRow` (malformed nested value, then
a valid row).
##########
arrow/array/struct.go:
##########
@@ -494,40 +525,59 @@ func (b *StructBuilder) UnmarshalOne(dec *json.Decoder)
error {
return errors.New("missing key")
}
- if keylist[key] {
+ if _, dup := keylist[key]; dup {
return fmt.Errorf("key %s is specified twice",
key)
}
- keylist[key] = true
+ var next json.RawMessage
+ if err := dec.Decode(&next); err != nil {
+ return err
+ }
- idx, ok := b.dtype.(*arrow.StructType).FieldIdx(key)
+ idx, ok := dtype.FieldIdx(key)
if !ok {
- var extra interface{}
- if err := dec.Decode(&extra); err != nil {
- return err
- }
continue
}
- if err := b.fields[idx].UnmarshalOne(dec); err != nil {
- return err
+ if bytes.Equal(next, []byte("null")) &&
!dtype.Field(idx).Nullable {
+ return fmt.Errorf("field '%s' is non-nullable
but got null", dtype.Field(idx).Name)
+ }
+
+ keylist[key] = next
+ }
+
+ // consume '}'
+ if _, err := dec.Token(); err != nil {
+ return err
+ }
+
+ // check that all non-nullable fields were specified
+ for _, field := range dtype.Fields() {
+ if _, ok := keylist[field.Name]; !ok && !field.Nullable
{
+ return fmt.Errorf("field '%s' is required but
no value was given", field.Name)
}
}
- // Append null values to all optional fields that were not
presented in the json input
- for _, field := range b.dtype.(*arrow.StructType).Fields() {
- if !field.Nullable {
+ // All validation passed; append the struct entry and its child
values.
+ b.Append(true)
+ for i, field := range dtype.Fields() {
+ next, hasKey := keylist[field.Name]
+ if !hasKey {
+ // Optional fields that were not present get a
null.
+ if field.Nullable {
+ b.fields[i].AppendNull()
+ }
continue
}
- idx, _ :=
b.dtype.(*arrow.StructType).FieldIdx(field.Name)
- if _, hasKey := keylist[field.Name]; !hasKey {
- b.fields[idx].AppendNull()
+
+ valDec := json.NewDecoder(bytes.NewReader(next))
+ valDec.UseNumber()
+ if err := b.fields[i].UnmarshalOne(valDec); err != nil {
+ b.Resize(-1)
Review Comment:
Fixed, and thanks for the precise diagnosis. `StructBuilder` no longer calls
`Resize(-1)`; it builds its own reusable `builderCheckpoint` (it needs one
because it is the root builder for `array.FromJSON` on a struct type, which
#1113 does not cover) and restores it on error.
`TestStructBuilderRollsBackRowAfterNestedDecodeError` decodes
`{"a":1,"b":[2,"bad"]}` followed by a valid row and asserts no panic, no
retained row, and correct output.
--
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]
