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
ARROW-2270: [Python] Fix lifetime of ForeignBuffer base object #1714
Conversation
93f85f9
to
37a9f78
Compare
// ---------------------------------------------------------------------- | ||
// Foreign buffer | ||
|
||
Status PyForeignBuffer::Make(const uint8_t* data, int64_t size, PyObject* base, |
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.
Can you add the description of the JIRA ticket somewhere in the code? I was not aware what went wrong in the initial implementation only from reading the change but I needed the context of the JIRA issue.
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.
Ok, I added an explanation.
37a9f78
to
217aa3a
Compare
(note that a separate Python-facing ForeignBuffer isn't really necessary anymore) |
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.
+1, thanks for spotting this memory "loophole".
Do we need to remove pyarrow.ForeignBuffer, then? |
The alternative would be to expose a factory function, e.g. |
Happy with the function. It could be nice to also access the base object later on again but currently I see no need for that. |
217aa3a
to
9e9c8c8
Compare
Ok, I did the change to the factory function. |
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.
+1
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.
+1, thanks @pitrou!
9e9c8c8
to
51f2d85
Compare
I added this new function to the API documentation. Merging |
No description provided.