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?

You can review commit by commit.

To address Issue 1:

• Make RecordBuilder.UnmarshalOne() return an error if a null is present for a non-nullable field, or if the value is not given.
• Same as above for StructBuilder.UnmarshalOne(), so that it works recursively.
• RecordBuilder.UnmarshalOne() now validates first and appends later, so that even if it needs to raise an error, the user can keep trying to read new records without compromising the state of the reader. Before it could be the case that it would only partially append a row, leaving the builder in an undefined state before reading the next row. I didn't do the validate then append change for StructBuilder since it relies on field.UnmarshalOne(), and there is no way of unmarshaling without appending. This is a TODO for improvement.

To address Issue 2:

• Added a check for dtype.Field(i).Nullable in the Struct and Record serializers to encode things as nil if arr.IsNull(i).
• PROBLEM: most GetOneForMarshal() implementations do a if a.IsNull(I { return nil } check, so this is somewhat pointless. I could change GetOneForMarshal() to mean "get one for marshal assuming it's non-nullable", but it changes one of the invariants of using this public method.

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.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, this makes the JSON decoder less lenient and validation stricter. So this could be a breaking change for users who are relying on the leniency of the JSON decoder.

[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 :)