fix(arrow/array): honor field nullability in comparison and JSON marshaling#918
fix(arrow/array): honor field nullability in comparison and JSON marshaling#918zeroshade wants to merge 18 commits into
Conversation
This commit separates the actual implementation from the public `*Equal` functions. Now, all the public API does is convert the `opts ...EqualOption` into an `opt equalOption` struct and pass it into the implementation. This is useful to avoid having to reconstruct back and forth between the two in nested call stacks. All the implementations care about is `equalOption`, and `EqualOption` remains as a convenient thing only for the public API.
This allows for 2 things: 1. Users can now explicitly pass into `EqualOption` if they want the comparison functions to compare the nullable buffer or not. 2. Struct and record comparison can change the value of the `nullable` option depending on `innerField.Nullable`, making the comparison semantically accurate.
Modified `StructBuilder` and `RecordBuilder` to append the empty default value (usually zero) to the inner field if it's marked as nullable and the consumed value is null.
The array implementations need a way of knowing whether to ignore the valids buffer or not. By default, it shouldn't ignore it if the array is being serialized by itself, like with `MarshalJSON()`. However, if the array is a part of a `Field` in a `RecordBatch` or `Struct`, then the value of the valids buffer might need to be ignored. This will be implemented in the next commit.
This commit fixes some tests that were implicitly setting `valid=false` on non-nullable fields, which now causes legitimate test failures when roundtripping to JSON.
record.go/struct.go: call UseNumber() on the per-field sub-decoder in the nullable decode path so large int64 keeps full precision (regression against apache#816, surfaced by the rebase). json_reader_test.go: pass the nullable flag to the two-arg GetOneForMarshal. internal/json/json_stdlib.go: add IsNullMessage to the tinygo/stdlib json variant (it was only added to the goccy build), fixing the TinyGo 'undefined: json.IsNullMessage' example build.
A non-nullable list/union/struct can legitimately hold nullable children, so equality and JSON serialization must honor each child/element field's own nullability rather than propagating the parent's. Previously the parent nullability was pushed down, so null child slots were compared and serialized by their arbitrary underlying bytes -- which round-trips inconsistently and breaks cross-language integration (union, nested_large_offsets) even when values are logically equal. compare.go: unions and run-end-encoded arrays have no top-level validity bitmap, so skip their top-level null-count/validity checks (and the all-null shortcut); recurse into list/struct/map/union children with the child field's nullability. union.go: SparseUnion/DenseUnion GetOneForMarshal use the selected child field's nullability and emit [typeID, null] for a null child so it round-trips as null.
Follow-up to the field-local nullability change (addresses review of 79cf180): the exact Equal path and the chunked/table helpers still used parent/top-level nullability. arrayEqualList/LargeList/ListView/LargeListView and arrayEqualFixedSizeList now recurse into elements with the element field's nullability (arrayEqualStruct was already field-local; arrayEqualMap delegates to arrayEqualList). chunkedEqual and chunkedApproxEqual now skip the top-level NullN check for unions and run-end-encoded arrays via hasTopLevelValidityBitmap, so Table comparisons don't reject logically-equal union/REE columns.
Addresses review of 6f92ce2: chunkedEqual called exported SliceEqual, which rebuilds default options and drops the caller's nullable setting, so TableEqual/ChunkedEqual with WithNullable(false) re-compared each chunk slice as nullable. Call the private sliceEqual(..., opt) instead, matching chunkedApproxEqual.
- array/compare.go: read the right-hand field from right.Schema() (not left.Schema()) in recordEqual, recordApproxEqual, tableEqual, and tableApproxEqual, so field/schema differences (name, nullability, metadata) between the two sides are actually detected instead of a field being compared against itself. - array/record_test.go: fix a malformed JSON map-entry literal (missing comma) in TestRecordBuilder. - ipc/metadata_test.go: TestUnrecognizedExtensionType now compares against the real preserved extension metadata (name arrow.uuid, empty serialized value). The previous placeholder only passed because record comparison ignored field metadata before the compare.go fix.
The right-schema comparison fix routed RecordEqual/RecordApproxEqual/ TableEqual/TableApproxEqual through Field.Equal, which also compares field and type metadata. That rejected logically-equal records differing only in metadata -- for example a PARQUET:field_id attached during a parquet round-trip -- breaking parquet/pqarrow tests such as TestForceLargeTypes. Introduce fieldEqualIgnoringMetadata (name + nullability + metadata- insensitive TypeEqual) and use it in the four comparison functions. It still detects the nullability differences the schema check was added for, while restoring the metadata-insensitive behavior these APIs had before. This makes the earlier metadata_test.go expected-metadata tweak unnecessary, so revert it.
- compare.go: drop the arrow.TypeEqual call from the schema-field check. TypeEqual is not metadata-insensitive for large-list / list-view element types (they fall through to reflect.DeepEqual or Field.Equal), so relying on it could still reject records that differ only in child-field metadata. Field type and values are already validated per column by baseArrayEqual, so the schema-field check now compares only name and nullability (renamed schemaFieldEqual). - ipc/metadata_test.go: use the real preserved extension metadata (arrow.uuid, empty serialized value) and assert on the read-back field metadata explicitly, since record comparison no longer inspects field metadata.
GetOneForMarshal previously grew a nullable bool parameter on the public arrow.Array interface, breaking external callers and custom Array implementations. Restore the single-argument GetOneForMarshal(i int) and move the field-local nullability behavior to a new optional exported interface, arrow.NullableMarshaler, with GetOneForMarshalNullable(i int, nullable bool). Containers (struct, sparse/dense union, dictionary, run-end-encoded, extension) and RecordToJSON propagate each field's Nullable flag through an unexported getOneForMarshalNullable helper that type-asserts to NullableMarshaler and falls back to GetOneForMarshal for arrays that do not implement it, preserving the previous validity-bitmap behavior. ExtensionArrayBase deliberately does not implement NullableMarshaler so that the optional interface is never promoted onto embedding extension arrays and cannot bypass a concrete type's GetOneForMarshal override. The helper instead handles the one field-local case that matters for plain extension arrays: a null slot in a non-nullable field serializes the storage value rather than JSON null, while still honoring any custom GetOneForMarshal that returns a non-null logical value.
abf60ef to
94644b3
Compare
Address review findings on incomplete nullability propagation: - List marshaling (List/LargeList/ListView/LargeListView/FixedSizeList): marshal child slices element-by-element via getOneForMarshalNullable so a non-nullable element field serializes underlying values instead of JSON null at null child slots. - Exact sparse/dense union equality: thread equalOption and compare the active child with its field-local nullability (matching the approximate path) instead of the default-option public SliceEqual. - Run-end-encoded equality: compare the values child using opt.nullable && ValueNullable, matching the marshaler so JSON round trips stay consistent, instead of propagating the parent field nullability to the values child.
arrow.TypeEqual ignores RunEndEncodedType.ValueNullable, so two REE arrays with matching run-ends/values types but differing ValueNullable can reach the value comparison. Deriving childOpt from only the left array made Equal asymmetric (order-dependent). AND both sides' ValueNullable so the derivation is order-independent; when the two types match (the common case) behavior is unchanged.
There was a problem hiding this comment.
Pull request overview
This PR updates Arrow Go’s array comparison and JSON (un)marshaling logic to honor field-level nullability from schemas (inner-nullability parity), so non-nullable fields ignore validity-bitmap differences and do not serialize JSON null when a slot is marked null.
Changes:
- Thread schema field
NullablethroughEqual/RecordEqual/TableEqual/ChunkedEqual(+ Approx variants) via a newWithNullableoption and nullable-aware equality plumbing. - Introduce optional
arrow.NullableMarshalerand propagate field nullability through containers andRecordToJSON, with safe fallback behavior. - Adjust JSON decoding for struct/record builders to handle explicit
nullin non-nullable fields, and update fixtures/tests to declare inner field nullability explicitly.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/json/json.go | Add IsNullMessage helper for raw JSON null detection (go-json build). |
| internal/json/json_stdlib.go | Add IsNullMessage helper for raw JSON null detection (stdlib/tinygo build). |
| arrow/ipc/metadata_test.go | Adjust extension metadata expectations; explicitly assert metadata preservation since RecordEqual ignores metadata. |
| arrow/ipc/cmd/arrow-ls/main_test.go | Update expected schema formatting to reflect explicit inner-field nullability. |
| arrow/internal/arrjson/arrjson_test.go | Update JSON schema fixtures to mark struct children as nullable. |
| arrow/internal/arrdata/arrdata.go | Update struct-record fixtures to declare inner fields nullable and align validity masks. |
| arrow/extensions/variant.go | Implement nullable-aware marshaling via GetOneForMarshalNullable. |
| arrow/extensions/uuid.go | Implement nullable-aware marshaling via GetOneForMarshalNullable. |
| arrow/extensions/uuid_test.go | Update schema to set UUID field nullable for tests. |
| arrow/extensions/json.go | Make JSON extension marshaling nullable-aware without breaking ValueJSON API. |
| arrow/extensions/bool8.go | Implement nullable-aware marshaling via GetOneForMarshalNullable. |
| arrow/compute/vector_sort_test.go | Update test schemas to explicitly set fields nullable where JSON inputs include nulls. |
| arrow/array/util.go | Propagate field nullability in RecordToJSON; add helpers for nullable-aware marshaling including list element handling. |
| arrow/array/util_test.go | Exercise JSON round-trip with both nullable and non-nullable schemas. |
| arrow/array/union.go | Propagate child field nullability through union marshaling and equality. |
| arrow/array/timestamp.go | Add nullable-aware marshaling and thread nullability through timestamp equality. |
| arrow/array/struct.go | Propagate child field nullability through struct marshaling and equality; handle explicit null on non-nullable fields during decoding. |
| arrow/array/struct_test.go | Add coverage for explicit null in required nested struct fields. |
| arrow/array/string.go | Add nullable-aware marshaling and thread nullability through string equality functions. |
| arrow/array/record.go | Handle explicit null for non-nullable fields during record decoding. |
| arrow/array/record_test.go | Expand record builder test for nullable/non-nullable fields and JSON round-trip behavior. |
| arrow/array/numeric_generic.go | Add nullable-aware marshaling for numeric/date/time arrays. |
| arrow/array/null.go | Add GetOneForMarshalNullable to align with optional nullable marshaler pattern. |
| arrow/array/map.go | Thread nullability option through map equality. |
| arrow/array/list.go | Add nullable-aware list marshaling; thread child element nullability through list equality. |
| arrow/array/interval.go | Add nullable-aware marshaling and thread nullability through interval equality. |
| arrow/array/float16.go | Add nullable-aware marshaling for Float16 arrays. |
| arrow/array/fixedsize_binary.go | Add nullable-aware marshaling and thread nullability through equality. |
| arrow/array/fixed_size_list.go | Add nullable-aware marshaling; thread child element nullability through equality. |
| arrow/array/extension.go | Thread nullability through extension equality; document why base doesn’t implement NullableMarshaler. |
| arrow/array/encoded.go | Add nullable-aware marshaling and thread value-nullability through run-end-encoded equality. |
| arrow/array/dictionary.go | Add nullable-aware marshaling and thread nullability through dictionary equality. |
| arrow/array/decimal.go | Add nullable-aware marshaling and thread nullability through decimal equality. |
| arrow/array/compare.go | Core change: introduce schema-level nullability/field checks for record/table comparisons; add WithNullable; plumb nullable through equality stack and special-case types without top-level validity. |
| arrow/array/compare_test.go | Add tests for equality semantics when treating fields as non-nullable. |
| arrow/array/boolean.go | Add nullable-aware marshaling and thread nullability through boolean equality. |
| arrow/array/binary.go | Add nullable-aware marshaling and thread nullability through binary equality. |
| arrow/array.go | Introduce optional NullableMarshaler interface on arrays (non-breaking). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
How Arrow C++ handles run-end-encoded value nullabilityFor context on the REE value-nullability discussion here, comparing against the C++ reference implementation: C++ has no configurable value nullability. children_ = {std::make_shared<Field>("run_ends", std::move(run_end_type), false),
std::make_shared<Field>("values", std::move(value_type), true)};
Implications:
|
|
Closing in favor of #833. As detailed in the comparison above, the The |
Rationale for this change
This is the inner-nullability parity work that was previously bundled into #558 (the
TimestampWithOffsetcanonical extension type). #558 has now been reduced to just the extension type, and this PR carries the comparison and JSON-marshaling changes on their own so they can be reviewed independently and more carefully.What changes are included in this PR?
arrow/array/compare.go: thread each field'sNullableflag throughEqual/RecordEqual/TableEqual/ChunkedEqual(and theApproxvariants) so that non-nullable fields ignore validity-bitmap differences; add aWithNullablefunctional option; compare record/table fields against the right-hand schema (not the left against itself) and ignore field metadata during record/table comparison.arrow.Arrayinterface: keep the single-argumentGetOneForMarshal(i int)and move the field-local behavior to a new optionalNullableMarshalerinterface withGetOneForMarshalNullable(i int, nullable bool), so external callers and customArrayimplementations are not broken. Containers (struct, union, dictionary, run-end-encoded, extension) andRecordToJSONpropagate each field's nullability through an unexported helper that falls back toGetOneForMarshalwhen the optional interface is not implemented.field.Nullablewhen serializing to JSON, and handle explicitnullfor a non-nullable field when reading.internal/json: add a helper to detect anullmessage.Relationship to #833
#833 pursues the same goal with a different strategy: it drops the comparison leniency (
WithNullable/equalOption) in favor of schema validation inValidate()to match Arrow C++, and checksIsValid()in the struct/record builders rather than introducingGetOneForMarshalNullable(). This PR preserves the earlier comparison-based approach. Reviewers should decide which direction to land; opening as a draft pending that decision.Are these changes tested?
Yes.
go test ./arrow/array/ ./arrow/extensions/ ./arrow/internal/arrjson/ ./arrow/compute/ ./arrow/ipc/...pass.Are there any user-facing changes?
Yes: comparison functions gain a
WithNullableoption and the array marshaling path gains an optionalNullableMarshalerinterface. The existingGetOneForMarshal(i int)signature is preserved.