feat: make FFI structs fields pub#9772
Conversation
| } | ||
|
|
||
| unsafe impl Send for FFI_ArrowArrayStream {} | ||
| unsafe impl Sync for FFI_ArrowArrayStream {} |
There was a problem hiding this comment.
Why is this change made? I didn't see any mention of it in the PR description and given there are a buch of pointers involved I am not sure if it is safe to share this data across threads 🤔
There was a problem hiding this comment.
Fair question, I should've mentioned this.
Notice that FFI_ArrowArray is already Sync:
https://github.com/ashdnazg/arrow-rs/blob/aaf160f76e92b838b917d5e4be62de693258405f/arrow-data/src/ffi.rs#L77
IMO since these are FFI objects, they're kind of "anything goes", because whatever language you give them to, can do whatever it wants with them, so disallowing it inside Rust seems unhelpful. In addition, what little safe API they have, seems to be thread-safe (no cells and the like).
This is not super important, I can leave it out if you prefer, but I think it does make sense.
There was a problem hiding this comment.
Can you please pull it into a separate PR that we can discuss and analyze by itself. Otherwise this PR is ready to go from my perspective
| } | ||
|
|
||
| unsafe impl Send for FFI_ArrowSchema {} | ||
| unsafe impl Sync for FFI_ArrowSchema {} |
There was a problem hiding this comment.
likewise here -- why is this necessary?
These are defined in the standard, so encapsulation is unnecessary.
|
rebased and removed |
alamb
changed the title
pub
|
🚀 |
# Which issue does this PR close? - Closes apache#9771 . # Are these changes tested? No need for additional tests. # Are there any user-facing changes? New fields are exposed.
2 participants
Which issue does this PR close?
Are these changes tested?
No need for additional tests.
Are there any user-facing changes?
New fields are exposed.