zeroshade commented on code in PR #833:
URL: https://github.com/apache/arrow-go/pull/833#discussion_r3769993393
##########
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:
`Resize(-1)` does not reliably discard this row. Nested builders can append
their outer entry before a child fails, leaving every top-level field at the
same length. For `{"a":1,"b":[2,"bad"]}`, the error leaves both columns at
length 1, so this call is a no-op and the rejected row contaminates later
output.
Please restore every builder to an explicit pre-row checkpoint rather than
inferring rollback from top-level length differences. Add a malformed
nested-value test followed by 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:
This rollback panics when all child builders are equally advanced. In that
case `columnLenRange` leaves `n == -1`, which is passed to child `Resize`
methods and eventually causes an out-of-range panic in `CountSetBits`.
A malformed list field such as `[2,"bad"]` reproduces this. Please restore
the parent and all descendants to their exact pre-row state and test that the
decode returns an error without retaining the row or panicking.
##########
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:
This validates only top-level schema fields. Nested field nullability is
still ignored; for example, a `ListOfNonNullable(int32)` accepts `[1,null]`
without error.
Please enforce nested field metadata consistently, or narrow the stated
compatibility scope if recursive list-element validation is intentionally
deferred. Add coverage for non-nullable list elements and corresponding nested
types.
##########
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:
This still serializes a null in a non-nullable column: when the condition is
false, the column’s `GetOneForMarshal` returns `nil`. The resulting
`{"x":null}` is then rejected by this PR’s reader, breaking writer/reader round
trips.
Please define and implement the intended behavior for invalid non-nullable
data—likely return an encoding error—and add a round-trip regression. The
equivalent struct path has the same issue.
--
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]
