Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ffi stream and utils from arrow to arrow-array #5670

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

alexandreyc
Copy link
Contributor

@alexandreyc alexandreyc commented Apr 19, 2024

Which issue does this PR close?

Closes #5663.

Rationale for this change

We have to depend on arrow for using the C Stream Interface for ADBC but we would prefer to depend on smaller crates. See apache/arrow-adbc#1725 (comment)

Are there any user-facing changes?

I don't think so but please double check.

Notes

  • I had to remove usage of compute kernels within tests because this introduces a dev-dependency cycle (arrow-array <-> arrow-arith) which is problematic for the Array trait. See https://doc.rust-lang.org/cargo/reference/resolver.html#dev-dependency-cycles

  • I had to redefine Result<T> = std::result::Result<T, ArrowError> because it's only defined in arrow. Maybe we should move it to arrow_schema alongside ArrowError.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 19, 2024
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I think it just needs a merge from master to fix the integration tests. As this changes the FFI code, I'd like them to run green prior to merge

@alexandreyc
Copy link
Contributor Author

alexandreyc commented Apr 22, 2024

CI is now green!

@tustvold tustvold merged commit ae9b0db into apache:master Apr 22, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract ffi_stream and to_ffi/from_ffi to a separate crate?
2 participants