-
Notifications
You must be signed in to change notification settings - Fork 727
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
ArrowArray::try_from_raw
should not assume the pointers are from Arc
#1334
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1334 +/- ##
=======================================
Coverage 83.03% 83.03%
=======================================
Files 181 181
Lines 52949 52951 +2
=======================================
+ Hits 43965 43968 +3
+ Misses 8984 8983 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. An Arrow array or schema can be allocated by other languages like Java (see here), so we shouldn't assume an Arc
here.
Merged, thanks @viirya ! |
Thanks @sunchao ! |
@@ -721,9 +721,11 @@ impl ArrowArray { | |||
.to_string(), | |||
)); | |||
}; | |||
let ffi_array = (*array).clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not super familiar with this code but it makes sense to me. The clone here seems to just be a clone of the FFI_ArrowArray
struct itself (which is some ints and pointers) which seems reasonable enough to me.
If someone seems performance issues from this code, we can always add a try_from_raw_arc
or something, but this looks good to me for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clone here seems to just be a clone of the FFI_ArrowArray struct itself (which is some ints and pointers) which seems reasonable enough to me.
That's right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alamb
Guys, not sure if my understanding is right, but I think this commit will break the design and create memory leak. If we clone the FFI struct, then it means we need to free the pointer by ourself, but if we free FFI_ArrowArray, then the data in this Array will also be free? Which means we can't free the pointer(until the data are used and ready to free, but in reality we can't hold this useless pointer in a big project for such a long time), which create memory leak. As to the question @viirya raised in #1333 , when manage memory, the one who allocate it should free it, which means in our case, we need to alloc the struct in rust and pass the pointer to java and then also free the memory in rust.
|
Simply said, I don't think this is correct. That is the whole point of C data interface. The release callback is designed to be called by not only the one allocates the C data interface structure. |
As the raw pointers are converted to |
…hema (apache#1334)" This reverts commit f84c436.
Which issue does this PR close?
Closes #1333.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?