serramatutu commented on code in PR #833:
URL: https://github.com/apache/arrow-go/pull/833#discussion_r3847193027
##########
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:
Defined it as an encoding error, as you suggested. `RecordToJSON` and
`Struct.MarshalJSON` (so `RecordBatch.MarshalJSON` too, via
`RecordToStructArray`) now return `arrow.ErrInvalid` when a non-nullable field
holds a null, recursively. You were right that the old branch was pointless —
`GetOneForMarshal` returns `nil` either way — so both no-op checks are
reverted. The check honors parent-validity priority, so a null child under a
null struct parent is not an error, matching what the encoders actually emit.
Covered by `TestRecordToJSONRejectsNullInNonNullableField`,
`TestStructMarshalJSONAllowsNullChildUnderNullParent` and a round-trip test.
One scope note: `MarshalJSON` on non-struct array roots (a bare list array,
say) still writes nulls for non-nullable element fields. Reading validates
those roots; writing does not. Happy to extend it here if you want it in this
PR.
##########
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++ {
Review Comment:
Enforced recursively now rather than narrowing the scope.
`arrow/array/nullability.go` walks a buffered value against the `arrow.Field`
tree, covering struct fields, the element field of list / large list / list
view / fixed-size list, map key and item fields, union children, dictionary
values, run-end-encoded values and extension storage.
`ListOfNonNullable(int32)` rejects `[1,null]`, and missing non-nullable fields
are rejected at any depth. Roots that are not records or structs are validated
in `FromJSON`, so `array.FromJSON(mem, arrow.ListOfNonNullable(...), ...)` is
checked too. `arrow/array/nullability_test.go` has the coverage.
Unexpected bonus: buffering the row made `BenchmarkRecordFromJSON/Size_1000`
~305x faster (7.77 s/op -> 25 ms/op), because decoding string tokens straight
off the document decoder is quadratic in goccy's `(*Stream).Token`. Details in
the PR description.
--
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]
