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

Use pyarrow.ffi to allocate intermediate C structures #3

Merged
merged 17 commits into from
Jan 23, 2022

Conversation

paleolimbot
Copy link
Collaborator

Following the discussion at #2, the way that pointers are allocated has changed in the Arrow R package master branch, with a release (7.0.0) coming soon. In particular, the internal functions allocate_arrow_*() and delete_arrow_*() now work with external pointers (EXTPTRSXP) rather than uintptr_t casted to double and wrapped in a numeric vector. The unit tests for the import/export interface use pyarrow.ffi to allocate and delete the intermediate struct ArrowArray/Schema/ArrayStream...I think it makes sense to do that here, too, and results in a slightly cleaner pattern (because cffi takes care of the deleting with garbage collection).

Happy to take a different approach if needed!

Some references that might be useful when reviewing:

paleolimbot and others added 7 commits January 18, 2022 09:01
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
- Ensure that correct R function `packageVersion` is called.
- Use single-quoted string in Python whenever possible
- Use alphabetical order in class name declaration in converter dict.
Copy link
Member

@lgautier lgautier left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for the PR.

@lgautier
Copy link
Member

One of commit messages says:

add recordbatchreader tests (except test that's segfaulting)

Can you open an issue if there is a test still segfaulting? Thanks.

@lgautier lgautier merged commit bf1ef13 into rpy2:main Jan 23, 2022
@lgautier lgautier mentioned this pull request Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants