GH-37072: [C++] MakeArrayOfNulls should respect Field::nullable

[bkietz]

Rationale for this change

MakeArrayOfNulls didn't examine Field::nullable, so it could produce nested arrays whose children were null even if the schema said they couldn't have nulls. Additionally validation didn't look at Field::nullable so these malformed arrays don't fail tests.

Are these changes tested?

Yes, mostly by fixtures already in place.

Behavior

This PR includes breaking changes to public APIs.

The Field::nullable flag is now enforce in array validation, so now (for example) an array with nulls which corresponds to a non nullable field will fail validation.

• Closes: null structarrays are poorly handled by cast #37072

[felipecrv]

Many tests are failing now.

[felipecrv]

Isn't a bit weird to have "Invalid: " twice in the message?

[bkietz]

It is, but this is the format of other child array validation failures (since the parent array's error message contains the child array's). Possibly a follow up issue is in order to simplify those; I think it'd be most valuable to provide a field path to the child which failed validation and the "leaf" error message.

[felipecrv]

Why delete this?

[bkietz]

I didn't really delete it, just refactored its logic into NullArrayFactory. This way the code which requests preallocated zero buffer is in the same function as the code which uses the zero buffer, which seemed more clear to me.

[pitrou]

Some comments, questions and suggestions below.

The PR description should note that this is a behavior change.

[felipecrv]

Getting merged and me rebasing the LIST_VIEW PR would be a good idea.

[pitrou]

Why not also override storage_type_ref?

[bkietz]

I intended to remove storage_type_ref altogether

[pitrou]

Shouldn't this be successful if length is 0?

[bkietz]

If the type cannot support any number of nulls, I would say that the special case of zero length is not worth allowing

[thisisnic]

Thank you for your contribution. Unfortunately, this
pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label
or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you
do not have repository permissions to reopen the PR, please tag a maintainer.

[thisisnic]

I'm going through old/abandoned PRs which would be advantageous to be able to merge and taking a look at ones which are mostly done and I can use AI to finish them off. This one looks mostly done so I did try to rebase and see how things look on CI. However, an AI review flagged up some missing things needed before merging, assuming the tests pass. Will paste below:

---

Rebase notes: conflicts were all mechanical. Upstream renamed HasValidityBitmap to may_have_validity_bitmap, moved logging.h and sort.h to logging_internal.h and sort_internal.h, and the parquet-testing submodule pointer had moved. storage_type_ref is gone as intended. Locally, arrow-array-test passes (1071 tests) and arrow-ipc-read-write-test passes apart from one test that needs a test-data file my checkout doesn't have, so unrelated. C++ format and lint hooks pass.

Findings from an AI review of the rebased diff. The first one I checked by hand and it's real. The rest I'm relaying as-is for whoever picks this up.

1. validate.cc, RecurseInto: when a field is non-nullable, full validation is on, and the child's null_count is unknown, related_validator.GetNullCount() runs before related_validator.Validate(). GetNullCount() reads data.buffers[0] and calls CountSetBits over a bitmap whose size hasn't been checked yet, so a malformed child can read out of bounds. The existing comment in Validate() ("Check nulls after validating the buffer sizes") describes exactly this ordering. Fix is to call Validate() first and only then check the null count.

2. validate.cc, RecurseInto: cheap Validate() now rejects children with nulls under a non-nullable field, not just ValidateFull(). That's a behaviour change visible to every binding, including IPC and Parquet reads that call Validate(), and there's no test asserting on the new "was non-nullable but had" error. Needs tests for struct, list, fixed-size list, union and run-end encoded children, and an explicit decision on whether cheap validation should enforce it.

3. util.cc, NullArrayFactory::Visit(const UnionType&) and Visit(const RunEndEncodedType&): both ignore nullable_ and raise TypeError when no member is nullable, even when the union or REE array is itself a non-nullable child that never needs to hold a null. So MakeArrayOfNull(struct_({field("", dense_union({req(int8())}), false)})) fails although the struct's own bitmap would mask the child. When !nullable_ the nullable-member requirement can be skipped.

4. util.cc, GetZeroBufferLength: the presizing pass wraps VisitTypeInline in DCHECK_OK, and the fallback Visit(const DataType&) returns NotImplemented, so an unsupported type now aborts in debug builds instead of returning a Status.

5. type.h and extension_type.h: storage_type() becomes virtual and returns by value, adding a vtable slot to DataType and a shared_from_this lock on every RecurseIntoField. Calling it on a DataType not owned by a shared_ptr throws bad_weak_ptr.

6. validate.cc, ValidateArrayImpl::GetNullCount: uses data.type->id() where neighbouring code uses storage_id(), so an extension type with a validity bitmap reports 0 nulls. The comment "Do not call GetNullCount()" now sits inside a method called GetNullCount and should say ArrayData::GetNullCount().

7. validate.cc, RecurseInto: st.WithMessage(description..., " invalid: ", st.ToString()) embeds the status prefix, giving "invalid: Invalid: ...". Using st.message() fixes it. This is the doubled prefix felipecrv asked about in the still-open thread on array_run_end_test.cc.

8. random.cc: the structured binding field in the struct case shadows the field parameter.

The one open design thread from the 2023 review is whether a zero-length array of a type that can't hold nulls should succeed (pitrou asked, bkietz preferred not to special-case it).

[thisisnic]

Closing as stale, I don't have capacity to finish this off, but feel free to reopen if anyone is working on it.