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.
##########
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.
##########
arrow/array/struct.go:
##########
@@ -207,9 +207,14 @@ func (a *Struct) GetOneForMarshal(i int) interface{} {
}
tmp := make(map[string]interface{})
- fieldList := a.data.dtype.(*arrow.StructType).Fields()
+ dtype := a.data.dtype.(*arrow.StructType)
+ fieldList := dtype.Fields()
for j, d := range a.fields {
- tmp[fieldList[j].Name] = d.GetOneForMarshal(i)
+ if dtype.Field(j).Nullable && a.IsNull(i) {
+ tmp[fieldList[j].Name] = nil
+ } else {
+ tmp[fieldList[j].Name] = d.GetOneForMarshal(i)
+ }
}
Review Comment:
Reverted this branch entirely — it was indeed unreachable, and even
reachable it would have been a no-op since `GetOneForMarshal` returns `nil` for
a null slot anyway. Non-nullable nulls are now rejected by `Struct.MarshalJSON`
instead of being re-encoded.
##########
arrow/array/struct.go:
##########
@@ -467,19 +472,27 @@ func (b *StructBuilder) UnmarshalOne(dec *json.Decoder)
error {
if keylist[key] {
return fmt.Errorf("key %s is specified twice",
key)
}
-
keylist[key] = true
- idx, ok := b.dtype.(*arrow.StructType).FieldIdx(key)
+ var next json.RawMessage
+ if err := dec.Decode(&next); err != nil {
+ return err
+ }
+
+ dtype := b.dtype.(*arrow.StructType)
+
+ 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 {
+ 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)
+ }
+
+ valDec := json.NewDecoder(bytes.NewReader(next))
+ valDec.UseNumber()
+ if err := b.fields[idx].UnmarshalOne(valDec); err !=
nil {
return err
Review Comment:
Fixed. Validation now runs on the buffered value before anything is
appended, and `StructBuilder` restores a pre-row `builderCheckpoint` if a child
decode fails, so the builder is never left advanced after an error.
##########
arrow/array/record_test.go:
##########
@@ -511,14 +516,23 @@ func TestRecordBuilder(t *testing.T) {
}
}
+ err := b.UnmarshalJSON([]byte(`{"f1-i32": null, "f2-f64-notnull": null,
"map": null}`))
+ assert.Contains(t, err.Error(), "field 'f2-f64-notnull' is non-nullable
but got null")
+
+ err = b.UnmarshalJSON([]byte(`{"f1-i32": null, "map": null}`))
+ assert.Contains(t, err.Error(), "field 'f2-f64-notnull' is required but
no value was given")
+
+ err = b.UnmarshalJSON([]byte(`{"f1-i32": 6, "f2-f64-notnull": 6.6,
"map": [{"key": "4": "value": "d"}]}`))
+ assert.NoError(t, err)
Review Comment:
Fixed — the missing comma is corrected.
##########
arrow/array/record_test.go:
##########
@@ -527,9 +541,27 @@ func TestRecordBuilder(t *testing.T) {
if got, want := rec.ColumnName(0), schema.Field(0).Name; got != want {
t.Fatalf("invalid column name: got=%q, want=%q", got, want)
}
- if got, want := rec.Column(2).String(), `[{["0" "2" "3"] ["a" "b" "c"]}
{[] []} {[] []} {["3" "2" "3"] ["a" "b" "c"]} {[] []}]`; got != want {
- t.Fatalf("invalid column name: got=%q, want=%q", got, want)
+
+ if got, want := rec.Column(0).String(), `[(null) 2 3 4 5 6]`; got !=
want {
+ t.Fatalf("invalid column values: got=%q, want=%q", got, want)
+ }
+ if got, want := rec.Column(1).String(), `[1.1 2.2 3.3 4.4 5.5 6.6]`;
got != want {
+ t.Fatalf("invalid column values: got=%q, want=%q", got, want)
}
+ if got, want := rec.Column(2).String(), `[{["0" "2" "3"] ["a" "b" "c"]}
{[] []} {[] []} {["3" "2" "3"] ["a" "b" "c"]} {[] []} {["4"] ["d"]}]`; got !=
want {
+ t.Fatalf("invalid column values: got=%q, want=%q", got, want)
+ }
+
+ // roundtripping from JSON with array.FromJSON should work
+ arr := array.RecordToStructArray(rec)
+ defer arr.Release()
+ jsonStr, err := json.Marshal(arr)
+ assert.NoError(t, err)
+
+ roundtripped, _, err := array.FromJSON(mem, arr.DataType(),
bytes.NewReader(jsonStr))
+ defer roundtripped.Release()
+ assert.NoError(t, err)
+ assert.Truef(t, array.Equal(arr, roundtripped), "JSON round trip
returns different array: got=%q, want=%d", arr, roundtripped)
Review Comment:
Fixed. `require.NoError` now runs before `defer roundtripped.Release()`, and
the format verb is `%q` for both arguments.
--
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]
