zeroshade commented on code in PR #833:
URL: https://github.com/apache/arrow-go/pull/833#discussion_r3866128854
##########
arrow/array/record.go:
##########
@@ -599,49 +601,73 @@ func (b *RecordBuilder) unmarshalOne(dec *json.Decoder)
(err 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 {
Review Comment:
**Blocking:** This still delegates the nested value directly to the child
builder, which has no access to the containing nested `arrow.Field.Nullable`
metadata. I reproduced `RecordFromJSON` accepting `{"x":[1,null]}` for a schema
whose field type is `ListOfNonNullable(int32)`. Please enforce nested
nullability recursively (including corresponding nested container types) and
add a regression that expects this input to fail.
##########
arrow/array/util.go:
##########
@@ -296,7 +296,11 @@ func RecordToJSON(rec arrow.RecordBatch, w io.Writer)
error {
cols := make(map[string]interface{})
for i := 0; int64(i) < rec.NumRows(); i++ {
for j, c := range rec.Columns() {
- cols[fields[j].Name] = c.GetOneForMarshal(i)
+ if rec.Schema().Field(j).Nullable && c.IsNull(i) {
Review Comment:
**Blocking:** This condition does not prevent invalid non-nullable nulls
from being serialized: when the field is non-nullable and `c.IsNull(i)` is
true, execution falls through to `GetOneForMarshal`, which returns `nil`. I
reproduced `RecordToJSON` succeeding with `{"x":null}`;
`Struct.GetOneForMarshal` likewise emits `[{"x":null}]`, while the updated
readers reject both. Please reject invalid non-nullable data during
serialization (or otherwise define consistent reader/writer behavior) and cover
both record and struct round trips.
--
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]
