GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values

[NickCrews]

• GitHub Issue: [Python] Cannot cast nested nullable field to not-nullable #33592

Notes for myself/fixer:

• tests that need to get updated (almost definitely not a complete list)
• [update: actually we should handle the go implementation in the go repository.] hmm, looks like go wrapper does its own nullability checks. I assume this is just an optimization to not have to go into the C++ and hit the error down there. So if we just delete this check then I think the C++ logic will handle all of it???
• how the plain column handles this cast, some logic like this probably needs to get ported over to the struct implementation
• (running from /cpp/build) cmake .. --preset ninja-debug-basic, then cmake --build . && PYTHON=python ctest -R 'arrow-compute-scalar-cast-test' --output-on-failure to run the specific test
• to format: uvx pre-commit run --all-files clang-format

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

⚠️ GitHub issue #33592 has been automatically assigned in GitHub to PR creator.

[rustyconover]

This PR looks pretty good to me, what additional help would you like getting it merged?

[NickCrews]

Thanks for the help! I'm a random contributor, so I'm not sure about the exact workflow, but I think both getting the CI run approved and having a C++ owner approve it are needed.

Any more detailed thoughts on if the tests are adequate, if there's anywhere else in the code base that you think needs to change, my handling of the go implementation by just deleting the shortcircuit, or any other more detailed thoughts? Basically anything that would reduce the mental load on the code owner I think would increase the odds they approve it :)

[mapleFU]

You can mark as ready for review? Or this is still wip?

[NickCrews]

Oops, didn't mean for this to still be marked WIP. Looks like a few failures that need to get fixed, but still a high-level review of the general approach would still be appreciated in the meantime.

[zeroshade]

You probably need the same corresponding check on the Go side to verify that it's only allowed if there are no nulls.

Also, the Go implementation has been moved to the apache/arrow-go repository. So please file the PR there for the Go side. Sorry for the confusion, we just haven't removed the Go code from here yet.

[NickCrews]

Thanks @zeroshade , will do. Do we need both PRs to land at about the same time, or can I do that independently?

I will undo my changes to the go code here, leaving it untouched.

[zeroshade]

They can land independently, no issues there. Thanks!

[NickCrews]

Just pushed a new version:

• dropped the Go changes
• rebased on top of 3600db8, which is one commit behind main, because main is failing CI, but the commit I chose passed CI. I did this because I think the failing CI checks in this PR are not related to this PR
• I discovered archery and formatted the files with archery lint --clang-format --fix

We will see if this passes CI now...

[NickCrews]

Pushed a new version, hopefully that fixed the broken tests:

• To go from nullable to non-nullable type, need to use the unsafe cast option
• fix typo of int8 to int64 so the precision matches
• removed the [Go] tag from the commit message

[NickCrews]

@zeroshade I think this is ready to review/merge, the failing CI runs look like flakes when trying to setup the environment?

[kou]

Can we move this to apache/arrow-go?

[kou]

Ah, the Go part was removed from this PR.
I've removed "[Go]" from the PR title.

[NickCrews]

The current CI failures look to be caused by #44985 (or similar), not by this PR.

I rebased on main, everything looks pretty much the same as before.

@zeroshade can you give this a review when you get the chance, or let me know how I can make it easier for you to do so? @rustyconover if you want to take a look again, maybe that will help. Thank you both!

[zanmato1984]

A small question about null count checking.

[mapleFU]

Besides, should we discard the buffer[0] if new data doesn't contains any nulls?

[NickCrews]

@mapleFU can you elaborate a little more? I don't think I understand neither why we should do this, or how. Does buffer[0] contain the null bitmap, and if there are no nulls then we can get away without storing one??

[mapleFU]

Ignore me, i previously think we can

        auto values = (in_array.child_data[field_index].ToArrayData()->Slice(
            in_array.offset, in_array.length));
       if (out field not nullable but in field nullable) {
          if (values->GetNullCount() > 0) {
             return error
          }
          values->buffers[0].reset();
       }

But I also think this doesn't matter here

[NickCrews]

@pitrou @mapleFU I applied your two suggestions, and I think this is ready for review again. Still one outstanding question about buffer[0]. CI is passing, except for one flaky test that failed during git checkout.

[mapleFU]

General LGTM!

[NickCrews]

whoops, I accidentally force-pushed with no changes, which closed the PR. Now it's back up

[NickCrews]

@mapleFU as I added the slicing tests, I also refactored the tests to make them much more consistent and concise. Take another look and make sure that your "LGTM" still holds after those changes.

[pitrou]

Just a nit!

[pitrou]

It's the field that's non-nullable

Suggested change

	return Status::Invalid("field '", in_field->name(), "' of type ",
	in_field_type->ToString(),
	" has nulls. Can't cast to field '", out_field->name(),
	"' of non-nullable type ", out_field_type->ToString());
	return Status::Invalid("field '", in_field->name(), "' of type ",
	in_field_type->ToString(),
	" has nulls. Can't cast to non-nullable field '",
	out_field->name(), "' of type ",
	out_field_type->ToString());