Skip to content

fix(arrow/array): honor field nullability in comparison and JSON marshaling#918

Closed
zeroshade wants to merge 18 commits into
apache:mainfrom
zeroshade:nullability-fixes
Closed

fix(arrow/array): honor field nullability in comparison and JSON marshaling#918
zeroshade wants to merge 18 commits into
apache:mainfrom
zeroshade:nullability-fixes

Conversation

@zeroshade

Copy link
Copy Markdown
Member

Rationale for this change

This is the inner-nullability parity work that was previously bundled into #558 (the TimestampWithOffset canonical 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's Nullable flag through Equal/RecordEqual/TableEqual/ChunkedEqual (and the Approx variants) so that non-nullable fields ignore validity-bitmap differences; add a WithNullable functional option; compare record/table fields against the right-hand schema (not the left against itself) and ignore field metadata during record/table comparison.
  • arrow.Array interface: keep the single-argument GetOneForMarshal(i int) and move the field-local behavior to a new optional NullableMarshaler interface with GetOneForMarshalNullable(i int, nullable bool), so external callers and custom Array implementations are not broken. Containers (struct, union, dictionary, run-end-encoded, extension) and RecordToJSON propagate each field's nullability through an unexported helper that falls back to GetOneForMarshal when the optional interface is not implemented.
  • Struct/record: honor field.Nullable when serializing to JSON, and handle explicit null for a non-nullable field when reading.
  • internal/json: add a helper to detect a null message.
  • Test fixtures updated to declare inner struct field nullability explicitly.

Relationship to #833

#833 pursues the same goal with a different strategy: it drops the comparison leniency (WithNullable/equalOption) in favor of schema validation in Validate() to match Arrow C++, and checks IsValid() in the struct/record builders rather than introducing GetOneForMarshalNullable(). 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 WithNullable option and the array marshaling path gains an optional NullableMarshaler interface. The existing GetOneForMarshal(i int) signature is preserved.

serramatutu and others added 12 commits July 9, 2026 15:37
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.
@zeroshade zeroshade marked this pull request as ready for review July 9, 2026 19:41
zeroshade added 4 commits July 9, 2026 15:44
- 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.
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Nullable through Equal/RecordEqual/TableEqual/ChunkedEqual (+ Approx variants) via a new WithNullable option and nullable-aware equality plumbing.
  • Introduce optional arrow.NullableMarshaler and propagate field nullability through containers and RecordToJSON, with safe fallback behavior.
  • Adjust JSON decoding for struct/record builders to handle explicit null in 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.

Comment on lines +558 to +561
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)
@zeroshade

Copy link
Copy Markdown
Member Author

How Arrow C++ handles run-end-encoded value nullability

For context on the REE value-nullability discussion here, comparing against the C++ reference implementation:

C++ has no configurable value nullability. RunEndEncodedType hard-wires the values child to nullable=true (type.cc#L1307):

children_ = {std::make_shared<Field>("run_ends", std::move(run_end_type), false),
             std::make_shared<Field>("values",   std::move(value_type),   true)};
  • DataType equality compares child data types only — the values field's nullability is not part of REE type identity (compare.cc#L883):
    result_ = left.value_type()->Equals(right.value_type()) &&
              left.run_end_type()->Equals(right.run_end_type());
  • Array equality merges runs and compares the decoded values (validity included, since values are always nullable); it never consults Field::nullable.
  • IPC reconstructs RunEndEncodedType(children[0]->type(), children[1]->type()) (metadata_internal.cc#L423) — the values-field nullable bit is dropped and re-fixed to nullable, so a non-nullable-values REE does not round-trip.
  • Spec (Columnar.rst): run_ends cannot be null; values may be null.

Implications:

  1. Excluding ValueNullable from arrow.TypeEqual (as it is today) matches C++. Making it part of type identity would be stricter than the reference and breaks REE record/IPC construction in arrow-go (schema-vs-array type mismatch, since ValueNullable isn't set consistently and doesn't survive IPC).
  2. More broadly, C++ never makes Equal() lenient based on a field's Nullable flag — it keeps equality strict and enforces schema nullability via Validate(). That is the direction taken in fix(arrow/array): Parity with Arrow C++ when reading/writing null to/from JSON #833, and it's why the equalOpts mechanism in this PR diverges from the reference. This branch remains useful as the record of the alternative approach, but the C++-aligned path is Validate()-based (fix(arrow/array): Parity with Arrow C++ when reading/writing null to/from JSON #833).

@zeroshade

Copy link
Copy Markdown
Member Author

Closing in favor of #833.

As detailed in the comparison above, the equalOpts leniency in this PR diverges from Arrow C++, which keeps Equal() strict and enforces schema nullability via Validate() instead. #833 implements that C++-aligned approach, so the inner-nullability parity work should land there.

The TimestampWithOffset canonical extension type that originally motivated this work has been decoupled into #558 and can merge independently.

@zeroshade zeroshade closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants