fix(arrow/array): Parity with Arrow C++ when reading/writing null to/from JSON

[serramatutu]

Rationale for this change

I raised an issue at the Arrow Community meeting regarding how Arrow Go allows the user to construct invalid record batches by reading JSON with nulls in it even if the schema says it's not null. It is also possible to construct invalid batches by calling AppendNull() in the builders, and currently there is no way to validate that. I was instructed to look at how Arrow C++/PyArrow do it, and replicate it here.

TL;DR:

1. Arrow C++ raises errors when reading JSON data that does not conform to the schema
2. Arrow C++ never JSON-encodes values as null if the parent field is non-nullable

Here's a link to my full investigation, including the code so you can run it yourself: https://github.com/serramatutu/arrow-internal-nulls

What changes are included in this PR?

Rebased on main, and reworked to address @zeroshade's review.

Reading (review point 4, plus the original issue 1). Nullability is now validated recursively, in one place, before anything is appended:

• arrow/array/nullability.go adds validateJSONNullability, which walks a buffered row against the arrow.Field tree. It covers struct fields, the element field of lists / large lists / list views / fixed-size lists, map key and item fields, union children, dictionary values, run-end-encoded values and extension storage types. So ListOfNonNullable(int32) now rejects [1, null], and a missing non-nullable field is rejected at any depth.
• RecordBuilder and StructBuilder keep upstream's streaming append path untouched. They only buffer the row and validate it when the schema declares a non-nullable field somewhere in its type tree (typeHasNonNullableField), so fully nullable schemas take exactly the same path as before this PR.
• Buffering the row also means a rejected row leaves the decoder positioned at the next row, so callers can keep reading. That is not achievable with streaming decode: an error raised inside a nested builder leaves the decoder at an unknown depth, which cannot be drained reliably.
• Roots that are not records or structs are validated in FromJSON, so array.FromJSON(mem, arrow.ListOfNonNullable(...), ...) is checked too.

Rollback (review points 1 and 2). #1113 landed while this PR was open and provides exactly the pre-row checkpoint the review asked for, so the Resize(-1) rollbacks are gone:

• RecordBuilder uses upstream's builderCheckpoint capture/restore as-is.
• StructBuilder now builds its own reusable builderCheckpoint, because it is the root builder for array.FromJSON on a struct type and had no rollback of its own. Resize(-1) is no longer used, and the panic the review reported (columnLenRange returning -1 into child Resize) is gone with it.

Writing (review point 3, plus the original issue 2). Writers now fail instead of emitting a null that this PR's reader would reject:

• RecordToJSON and Struct.MarshalJSON (and therefore RecordBatch.MarshalJSON, which goes through RecordToStructArray) return arrow.ErrInvalid when a non-nullable field holds a null.
• The check honors parent-validity priority, so a null child under a null struct parent is not an error — that matches what the encoders actually emit, since they stop descending at a null parent.
• The previous dtype.Field(i).Nullable && IsNull(i) branches were no-ops (GetOneForMarshal returns nil for a null slot either way), so they were reverted.

I had to change a bunch of tests that implicitly depended on fields being nullable, even if they didn't declare the fields as such.

Scope

MarshalJSON on non-struct array types (a list array marshalled on its own, say) still emits nulls for non-nullable element fields. Reading is validated for those roots; writing is not. Happy to extend it if you'd like it in this PR.

Are these changes tested?

Yes — arrow/array/nullability_test.go covers nested list/map/union/struct element nullability, root-level FromJSON, writer errors, parent-validity priority, decoding the row after a rejected row, and a JSON round trip over a schema mixing nullable and non-nullable fields. There is also a regression test for the reported StructBuilder panic (a malformed nested list value followed by a valid row).

go test ./arrow/... ./parquet/... passes, with the arrow_json_stdlib backend as well.

Performance

Buffering the row turned out to be a large speed-up, not a cost. go test ./arrow/array -run '^$' -bench 'BenchmarkRecordFromJSON/Size_1000$' -benchtime=3x -count=2 -benchmem (that benchmark's schema declares all five fields non-nullable, so it takes the new path):

	ns/op	B/op	allocs/op
main (b3dacd2a)	7,769,030,778 / 7,836,212,958	6,141,082	16,130
this branch	26,246,764 / 25,082,431	29,142,202	53,071

That is ~305x faster for ~4.7x the allocated bytes. A CPU profile of the old path attributes 98% of samples to runtime.memmove under goccy/go-json's decodeEscapeString ← (*Stream).Token: decoding string tokens one at a time straight off the document decoder is quadratic in the remaining buffer, so it gets worse the larger the document. Decoding each row from its own small json.RawMessage avoids that entirely.

The flip side: a schema with no non-nullable field anywhere still takes the streaming path and so keeps the quadratic behaviour (~7.8 s/op on the same data). I kept the fast path opt-in by nullability to keep this PR's behaviour change scoped — an unconditional row buffer would fix it for every schema, but it would also force UseNumber on caller-supplied decoders in UnmarshalOne. Happy to do that here, or file it separately, whichever you prefer.

Are there any user-facing changes?

Yes, two breaking changes:

1. The JSON decoder is stricter: null or a missing value for a non-nullable field is now an error, at any nesting depth. This breaks callers relying on the decoder's leniency.
2. The JSON encoders now return an error for arrays that hold a null in a non-nullable field, instead of silently writing null.

[zeroshade]

Is it possible for us to do this without a breaking change? I'd rather avoid the breaking change of changing a public API method if at all possible.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

arrow/array/record.go:424

• The UnmarshalOne docstring says it “receives an already-configured json.Decoder, so options such as UseNumber set by the caller are honored”, but the implementation now buffers values as RawMessage and re-decodes them with a fresh decoder that always calls UseNumber. This no longer honors caller decoder configuration (and in particular forces UseNumber even if the caller didn’t enable it).

// UnmarshalOne reads one row (a JSON object) from the supplied decoder and
// appends a value to each field in the RecordBuilder.
//
// Unlike UnmarshalJSON, this method receives an already-configured
// json.Decoder, so options such as UseNumber set by the caller are honored
// for nested field decoding. This is critical for preserving large integer
// values (>2^53) that cannot be represented exactly as float64.

[zeroshade]

@serramatutu are you still working on this?

[serramatutu]

@zeroshade I haven't had the time to push this forward since I last worked on it. Currently on PTO, will be back in July.

[serramatutu]

@zeroshade I addressed all the issues in new commits and rebased onto main. I don't have permissions to request another Copilot review. I did run a Claude Code review locally and it says it's good.

There is one issue that I found where b.Resize(-1) can leak memory which is causing a test to break. This was a pre-existing bug. I'm making a PR to fix that here. Will need to rebase onto that to make that test pass :)

[serramatutu]

@zeroshade I've rebased this onto the latest main and the tests are now passing :)

[zeroshade]

The stricter top-level nullability checks are directionally correct, but four correctness issues remain:

1. RecordBuilder rollback can retain rejected nested rows when top-level lengths remain equal.
2. StructBuilder rollback can pass -1 into child builders and panic.
3. Record and struct writers still emit non-nullable nulls that the new readers reject, breaking round trips.
4. Nested field metadata remains unenforced; for example, non-nullable list elements still accept null.

See the inline comments on record.go, struct.go, and util.go.

The existing array tests pass, as do focused race tests and all 23 CI checks. The PR is currently conflicting with main and will also require a rebase.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. After you've addressed the points above and pushed an update, an Apache Arrow Go maintainer — a real person — will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.

More on how Apache Arrow Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

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.

[serramatutu]

Per this: https://github.com/serramatutu/arrow-internal-nulls

In C++, it looks like the general approach is "validate all data in", then assume it's OK/consistent when "writing out". I.e it is an invariant that the data is correct when writing out.

If the user has messed around with the data:

• by tweaking memory directly: they need to ensure their code respects invariants OR call ValidateFull
• by using a public API: the API will always leave the array in a consistent internal state

This is my rationale for calling GetOneForMarshal() here: we assume the data is correct, if it's not then it's UB.

If we're very pedantic about this, we should just revert this change and use c.GetOneForMarshal(i) as we should assume IsNull(i) will always return false if the data is consistent with the schema. This is what C++ does actually, so here we're doing even more validation by checking rec.Schema().Field(j).Nullable. If this is writing wrong data that is failing validation upstream, I think we should fix the thing that allows the bad data to be constructed in the first place, not validate here while writing.

Sources:

• JsonWriter::WriteArray uses an ArrayWriter: https://github.com/apache/arrow/blob/b12755a1cc41ee5e9b1874ece3bcc92d6d3e3e78/cpp/src/arrow/integration/json_internal.cc#L2079
• ArrayWriter::WriteDataValues does not check for the schema at all, it just calls IsValid() directly: https://github.com/apache/arrow/blob/b12755a1cc41ee5e9b1874ece3bcc92d6d3e3e78/cpp/src/arrow/integration/json_internal.cc#L505

[zeroshade]

The checkpoint-based rollback changes resolve the prior record/struct contamination and panic findings. Two previously reported correctness gaps remain: nested nullability metadata is still not enforced, and record/struct serialization still emits nulls for non-nullable fields that the updated readers reject. Targeted, race, repeated rollback, impacted-package, formatting, vet, and diff checks pass. The PR is also still conflicting with main and currently has no reported CI checks.

[zeroshade]

Blocking: This still delegates the nested value directly to the child builder, which has no access to the containing nested arrow.Field.Nullable metadata. I reproduced RecordFromJSON accepting {"x":[1,null]} for a schema whose field type is ListOfNonNullable(int32). Please enforce nested nullability recursively (including corresponding nested container types) and add a regression that expects this input to fail.

[serramatutu]

Sorry, I was still fixing this yesterday but it got late...

I just fixed it for list, map, union and REE, and added tests for all of those. I also made all of them have individual checkpointing so if users use e.g a standalone ListBuilder with a non-nullable field, it'll still keep a consistent state.

Note that I had to add a new NewListBuilderWithField (also for list view and large lists) so that the builder can know the schema it's trying to build.

[zeroshade]

Thanks for the follow-up. The new commits fix the nested-nullability and rollback issue; the focused reproducer and array tests now pass.

1 blocker remains:

The current implementation buffers and re-decodes every record field, including fully nullable schemas. A direct benchmark against the PR base showed approximately 3.2x slower decoding, 9.5x more allocated bytes, and 4x more allocations. The nullable fast path described in the PR is not present in the pushed code.

The branch also currently conflicts with main, and no CI runs exist for the current head.

[serramatutu]

Worth noting: there was a recent change that introduced this test, which was silently decoding 1.5 into an Int32 array and casting it to 1. I assume this is a correctness behavior, and since this PR is about validating schemas, the JSON decoder now raises an error for that case. I changed the expected value to 2 so it's the appropriate type.

[serramatutu]

I had to add this due to bad performance from goccy when escaping newlines...

[serramatutu]

I added this as a way to reuse code between struct.go and builder.go since they were more or less doing the same logic when unmarshaling JSON objects into a list of field builders.