GH-14875: [C++] C Data Interface: check imported buffer for non-null

[pitrou]

The C data interface may expose null data pointers for zero-sized buffers.
Make sure that all buffer pointers remain non-null internally.

Followup to GH-14805

• Closes: [Python][C++] C Data Interface incorrect validate failures #14875

[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 old issues on JIRA the title also supports:

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

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[pitrou]

@zeroshade @alippai @alamb @viirya @tustvold Would it be easy to try this against the Go and Rust Arrow implementations?

[alamb]

Sorry @pitrou I am not sure how to test this against Rust. Did you have anything specific have in mind beyond the existing integration CI check https://github.com/apache/arrow/actions/runs/3595507889/jobs/6055082178 ?

[pitrou]

@alamb AFAIR there are Rust-PyArrow integration tests through the C Data Interface, no?

[wjones127]

@pitrou I think we just need to add a test case to this file roundtripping an empty array, and then test that against PyArrow built off of this branch, correct?

If so, I can do that.

[pitrou]

@wjones127 That would probably work, yes.

[wjones127]

I added tests here: apache/arrow-rs#3276

And locally tested them against PyArrow built from this branch. All tests pass 👍

[zeroshade]

@pitrou I'll build a pyarrow off this branch and test a roundtrip of an empty array between pyarrow and Go via C data. I'll comment again after I set it up.

[zeroshade]

@pitrou when I do round trips of the empty arrays, after fixing the crash in Go, I run into a few issues with pyarrow upon importing the empty array from C:

The data types: pa.binary(), pa.large_binary(), pa.string(), pa.large_string() all throw an error when calling validate(full=True) after the _import_from_c that contained a null value data buffer:

Traceback (most recent call last):
  File "/home/zeroshade/Projects/GitHub/arrow/go/arrow/cdata/test/test_export_to_cgo.py", line 218, in test
    b.validate(full=True)
  File "pyarrow/array.pxi", line 1501, in pyarrow.lib.Array.validate
  File "pyarrow/error.pxi", line 100, in pyarrow.lib.check_status
pyarrow.lib.ArrowInvalid: Value data buffer is null

I also ran into other cases where it through an error in _import_from_c because of null values but I think those are my fault, I'm going to double check and try again. But I'm pretty sure those validate's shouldn't be failing in this case if we're saying it's acceptable for a 0-length array to contain a null value data buffer.

[pitrou]

@zeroshade Nice catch, can you open an issue?

[zeroshade]

@pitrou I've opened issue #14875 for that. Gonna open a separate issue to fix the Go crash and see if the other failures are Go's fault or not.

[zeroshade]

I've opened #14877 to fix the Go side of these, though some of the python tests have to be commented out until #14875 is resolved.

[pitrou]

@zeroshade This draft PR now passes the uncommented Go tests (assuming you install the corresponding PyArrow version).

[github-actions]

• Closes: [Python][C++] C Data Interface incorrect validate failures #14875

[github-actions]

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

[zeroshade]

@pitrou if this passes the tests now, would it make sense to uncomment them in this PR? Or should we do that in a separate PR?

[pitrou]

@zeroshade We can't uncomment them until a PyArrow wheel is released that incorporates the fix, AFAIU.

[pitrou]

@paleolimbot FYI

[ursabot]

Benchmark runs are scheduled for baseline = 627caf3 and contender = 7319250. 7319250 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.51% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️1.74% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 73192505 ec2-t3-xlarge-us-east-2
[Failed] 73192505 test-mac-arm
[Finished] 73192505 ursa-i9-9960x
[Finished] 73192505 ursa-thinkcentre-m75q
[Finished] 627caf3f ec2-t3-xlarge-us-east-2
[Failed] 627caf3f test-mac-arm
[Finished] 627caf3f ursa-i9-9960x
[Finished] 627caf3f ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java