Improve C Data Interface and Add Integration Testing Entrypoints by pitrou · Pull Request #5080 · apache/arrow-rs

pitrou · 2023-11-15T22:59:32Z

Which issue does this PR close?

Closes #4867.

Rationale for this change

Allow Rust to participate in integration testing for the C Data Interface, as explained here:
https://arrow.apache.org/docs/dev/format/Integration.html#example-c-data-interface

What changes are included in this PR?

Add C-compatible entrypoints for the C Data Interface integration testing machinery.

Add support for exporting and importing Intervals over the C Data Interface.

Allow importing from a FFI_ArrowArray and an existing DataType.

Fix handling of null_count field in the C ArrowArray struct.

Are there any user-facing changes?

No.

pitrou · 2023-11-15T23:14:49Z

For now I have tested this locally against the C++ implementation, using these changes to the Archery machinery:
apache/arrow@main...pitrou:arrow:rust-cdata-integration

arrow-integration-testing/src/lib.rs

pitrou · 2023-11-15T23:29:21Z

arrow-integration-testing/src/lib.rs

This pattern would probably benefit from an API function. It's easy to get it wrong.

tustvold

Thank you for working on this, left some comments

arrow-integration-testing/src/lib.rs

pitrou · 2023-11-16T10:47:30Z

Ok, I've now checked this works properly against C++, Go, Java and C#. Will mark ready for review.

pitrou · 2023-11-16T10:49:19Z

@tustvold Is it possible to query the amount of memory allocated by Arrow Rust? It is not mandatory but allows checking for memory leaks during integration testing.

tustvold · 2023-11-16T11:03:22Z

Not easily afaik, you could register a global allocator that supports this, but nothing out of the box. Rust's custom allocators are not yet stable

pitrou · 2023-11-17T14:20:19Z

Not easily afaik, you could register a global allocator that supports this, but nothing out of the box. Rust's custom allocators are not yet stable

Ok, I see. We can revisit this later.

pitrou · 2023-11-17T15:06:28Z

@tustvold What would you think of adding a macro like this to ease error reporting?
https://gist.github.com/pitrou/698144c6277d913448398c996aaac3eb

tustvold · 2023-11-17T15:20:19Z

What would you think of adding a macro like this to ease error reporting?

No objections from me, although FYI #2725 tracks the broader issue that the error variants in arrow-rs are not particularly meaningful.

tustvold

This is looking good, mostly just minor nits, although I think we should probably make from_ffi and from_ffi_and_data_type unsafe before merging

tustvold · 2023-11-17T15:25:14Z

arrow-integration-testing/src/lib.rs

Perhaps we could get a doc comment explaining how this differs from ArrowFile? Perhaps LazyArrowFile might be a better name??

Alternatively I wonder if we could just make ArrowFile lazy, and have member functions for decoding a batch to RecordBatch by index or something

I've renamed to LazyArrowFile, which indeed sounds better. As for ArrowFile, it's marked public, is it ok to change its API?

This crate isn't published, so you can go wild 😄

Ok, I did the suggested changes.

arrow/src/ffi.rs

pitrou · 2023-11-20T12:01:21Z

arrow/src/ffi_stream.rs


            let schema_ref = self.schema();
+            // NOTE: this parses the FFI_ArrowSchema again on each iterator call;
+            // should probably use from_ffi_and_data_type() instead.


Perhaps a separate issue should be opened for this. @tustvold

pitrou · 2023-11-20T14:44:57Z

I think I addressed all review comments now.

tustvold · 2023-11-20T16:20:19Z

Thanks once again 👍

…n Rust (#38799) ### Rationale for this change Arrow Rust has added entrypoints for C Data Interface integration testing, so this can now be enabled on our side: apache/arrow-rs#5080 ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: #38798 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

…ting on Rust (apache#38799) ### Rationale for this change Arrow Rust has added entrypoints for C Data Interface integration testing, so this can now be enabled on our side: apache/arrow-rs#5080 ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38798 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

Arrow needs to be upgraded alongside pyo3 because they both link to python. Arrow's upgrade to pyo3 is in master but not yet released as of v51. apache/arrow-rs#5566 NOTE: Arrow marked `ffi::from_ffi` unsafe. apache/arrow-rs#5080

Arrow needs to be upgraded alongside pyo3 because they both link to python. NOTE: Arrow marked `ffi::from_ffi` unsafe. apache/arrow-rs#5080

github-actions bot added the arrow Changes to the arrow crate label Nov 15, 2023

pitrou commented Nov 15, 2023

View reviewed changes

arrow-integration-testing/src/lib.rs Outdated Show resolved Hide resolved

pitrou commented Nov 15, 2023

View reviewed changes

tustvold reviewed Nov 16, 2023

View reviewed changes

arrow-integration-testing/src/lib.rs Outdated Show resolved Hide resolved

arrow-integration-testing/src/lib.rs Outdated Show resolved Hide resolved

pitrou marked this pull request as ready for review November 16, 2023 10:47

pitrou force-pushed the cdata-integration-testing branch from 37f87cd to ddb35fb Compare November 17, 2023 14:19

tustvold approved these changes Nov 17, 2023

View reviewed changes

pitrou added 8 commits November 20, 2023 12:31

Add C Data Interface integration testing entrypoints

9f68695

Allow importing FFI_ArrowArray with existing datatype

956ba2e

Clippy

3ea2f9f

Use ptr::write

6dd48f1

Fix null_count for Null type

44c5184

Use new from_raw() APIs

b1c02e4

Address some review comments.

dcbf4f8

Add unsafe markers

4deebe6

pitrou force-pushed the cdata-integration-testing branch from ddb35fb to 4deebe6 Compare November 20, 2023 11:59

pitrou commented Nov 20, 2023

View reviewed changes

pitrou added 2 commits November 20, 2023 14:56

Try to fix CI

4812d7c

Revamp ArrowFile

69c2715

pitrou force-pushed the cdata-integration-testing branch from 674736f to 69c2715 Compare November 20, 2023 14:36

tustvold approved these changes Nov 20, 2023

View reviewed changes

tustvold added the api-change Changes to the arrow API label Nov 20, 2023

tustvold changed the title ~~Add C Data Interface integration testing entrypoints~~ Improve C Data Interface and Add Integration Testing Entrypoints Nov 20, 2023

tustvold merged commit b724849 into apache:master Nov 20, 2023

pitrou deleted the cdata-integration-testing branch November 20, 2023 16:58

pitrou restored the cdata-integration-testing branch November 20, 2023 16:58

pitrou deleted the cdata-integration-testing branch November 20, 2023 16:58

pitrou restored the cdata-integration-testing branch November 20, 2023 16:58

pitrou deleted the cdata-integration-testing branch November 20, 2023 16:58

This was referenced Nov 20, 2023

[Integration] Enable C Data Interface integration testing on Rust apache/arrow#38798

Closed

GH-38798: [Integration] Enable C Data Interface integration testing on Rust apache/arrow#38799

Merged

[Integration] Track Rust-allocated memory apache/arrow#38822

Open

Michael-J-Ward added a commit to Michael-J-Ward/dora that referenced this pull request Jun 7, 2024

deps: upgrade pyo3 and arrow deps

9458dff

Arrow needs to be upgraded alongside pyo3 because they both link to python. NOTE: Arrow marked `ffi::from_ffi` unsafe. apache/arrow-rs#5080

Michael-J-Ward added a commit to Michael-J-Ward/dora that referenced this pull request Jun 7, 2024

deps: upgrade pyo3 and arrow deps

cd8ed19

Arrow needs to be upgraded alongside pyo3 because they both link to python. NOTE: Arrow marked `ffi::from_ffi` unsafe. apache/arrow-rs#5080

Conversation

pitrou commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

pitrou commented Nov 15, 2023

Uh oh!

Uh oh!

pitrou Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pitrou commented Nov 16, 2023

Uh oh!

pitrou commented Nov 16, 2023

Uh oh!

tustvold commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Nov 17, 2023

Uh oh!

pitrou commented Nov 17, 2023

Uh oh!

tustvold commented Nov 17, 2023

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

tustvold Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

tustvold Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou commented Nov 20, 2023

Uh oh!

tustvold commented Nov 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pitrou commented Nov 15, 2023 •

edited

Loading

tustvold commented Nov 16, 2023 •

edited

Loading