[GitHub] [arrow] wesm commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests
wesm commented on a change in pull request #7753: URL: https://github.com/apache/arrow/pull/7753#discussion_r454493669 ## File path: python/pyarrow/jvm.py ## @@ -28,24 +28,38 @@ import pyarrow as pa -def jvm_buffer(arrowbuf): +class _JvmBufferNanny: +""" +An object that keeps a org.apache.arrow.memory.ArrowBuf's underlying +memory alive. +""" + +def __init__(self, jvm_buf): +self.jvm_buf = jvm_buf +self.jvm_buf.retain() + +def __del__(self): +self.jvm_buf.release() + + +def jvm_buffer(jvm_buf): """ Construct an Arrow buffer from org.apache.arrow.memory.ArrowBuf Parameters -- -arrowbuf: org.apache.arrow.memory.ArrowBuf +jvm_buf: org.apache.arrow.memory.ArrowBuf Arrow Buffer representation on the JVM. Returns --- pyarrow.Buffer Python Buffer that references the JVM memory. """ -address = arrowbuf.memoryAddress() -size = arrowbuf.capacity() -return pa.foreign_buffer(address, size, base=arrowbuf) +address = jvm_buf.memoryAddress() Review comment: I'm mostly trying to understand where and when this buffer will be destroyed. In any case it's outside the scope of this PR so we can let someone more invested in this code deal with it later This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests
wesm commented on a change in pull request #7753: URL: https://github.com/apache/arrow/pull/7753#discussion_r454493669 ## File path: python/pyarrow/jvm.py ## @@ -28,24 +28,38 @@ import pyarrow as pa -def jvm_buffer(arrowbuf): +class _JvmBufferNanny: +""" +An object that keeps a org.apache.arrow.memory.ArrowBuf's underlying +memory alive. +""" + +def __init__(self, jvm_buf): +self.jvm_buf = jvm_buf +self.jvm_buf.retain() + +def __del__(self): +self.jvm_buf.release() + + +def jvm_buffer(jvm_buf): """ Construct an Arrow buffer from org.apache.arrow.memory.ArrowBuf Parameters -- -arrowbuf: org.apache.arrow.memory.ArrowBuf +jvm_buf: org.apache.arrow.memory.ArrowBuf Arrow Buffer representation on the JVM. Returns --- pyarrow.Buffer Python Buffer that references the JVM memory. """ -address = arrowbuf.memoryAddress() -size = arrowbuf.capacity() -return pa.foreign_buffer(address, size, base=arrowbuf) +address = jvm_buf.memoryAddress() Review comment: I'm mostly trying to understand where and when this buffer will be deallcocated. In any case it's outside the scope of this PR so we can let someone more invested in this code deal with it later This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests
wesm commented on a change in pull request #7753: URL: https://github.com/apache/arrow/pull/7753#discussion_r454488716 ## File path: python/pyarrow/jvm.py ## @@ -28,24 +28,38 @@ import pyarrow as pa -def jvm_buffer(arrowbuf): +class _JvmBufferNanny: +""" +An object that keeps a org.apache.arrow.memory.ArrowBuf's underlying +memory alive. +""" + +def __init__(self, jvm_buf): +self.jvm_buf = jvm_buf +self.jvm_buf.retain() + +def __del__(self): +self.jvm_buf.release() + + +def jvm_buffer(jvm_buf): """ Construct an Arrow buffer from org.apache.arrow.memory.ArrowBuf Parameters -- -arrowbuf: org.apache.arrow.memory.ArrowBuf +jvm_buf: org.apache.arrow.memory.ArrowBuf Arrow Buffer representation on the JVM. Returns --- pyarrow.Buffer Python Buffer that references the JVM memory. """ -address = arrowbuf.memoryAddress() -size = arrowbuf.capacity() -return pa.foreign_buffer(address, size, base=arrowbuf) +address = jvm_buf.memoryAddress() Review comment: Do we expect that this is a "borrowed" reference here? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests
wesm commented on a change in pull request #7753: URL: https://github.com/apache/arrow/pull/7753#discussion_r454390287 ## File path: python/pyarrow/jvm.py ## @@ -43,9 +43,14 @@ def jvm_buffer(arrowbuf): pyarrow.Buffer Python Buffer that references the JVM memory. """ +import jpype address = arrowbuf.memoryAddress() size = arrowbuf.capacity() -return pa.foreign_buffer(address, size, arrowbuf.asNettyBuffer()) + +# TODO: why can we not use arrowbuf as the base? Review comment: I wasn't sure if there might be some kind of memory leak issue, @xhochy would know more since Uwe wrote this code originally This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests
wesm commented on a change in pull request #7753: URL: https://github.com/apache/arrow/pull/7753#discussion_r454378016 ## File path: python/pyarrow/jvm.py ## @@ -43,9 +43,14 @@ def jvm_buffer(arrowbuf): pyarrow.Buffer Python Buffer that references the JVM memory. """ +import jpype address = arrowbuf.memoryAddress() size = arrowbuf.capacity() -return pa.foreign_buffer(address, size, arrowbuf.asNettyBuffer()) + +# TODO: why can we not use arrowbuf as the base? Review comment: I think it has something to do with allocator reference counting but it would be good to know This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
