[GitHub] [arrow] rymurr commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests

2020-07-14 Thread GitBox 


rymurr commented on a change in pull request #7753:
URL: https://github.com/apache/arrow/pull/7753#discussion_r454498500



##
File path: ci/scripts/python_test.sh
##
@@ -29,4 +29,4 @@ export LD_LIBRARY_PATH=${ARROW_HOME}/lib:${LD_LIBRARY_PATH}
 # Enable some checks inside Python itself
 export PYTHONDEVMODE=1
 
-pytest -r s --pyargs pyarrow
+pytest -r s --pyargs pyarrow.tests.test_jvm

Review comment:
   Did this sneak in on accident?





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] rymurr commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests

2020-07-14 Thread GitBox 


rymurr commented on a change in pull request #7753:
URL: https://github.com/apache/arrow/pull/7753#discussion_r454427669



##
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:
   @wesm ref counting is a possibility. Though I don't see anywhere where 
the ref count of the netty java object is explicitly referenced.  
   
   When the python buffer object is garbage collected there is nothing (as far 
as I can tell) to collect the java based buffer. When the Python object is 
collected it will decrement the python reference count of the java object and 
eventually GC it but we would still have to manually close the Java side buffer 
regardless of it being wrapped in netty or not. So we would have a memory leak 
regardless if we don't carefully clean up buffers. Unless jpype does something 
cool and undocumented w/ Netty buffers.





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] rymurr commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests

2020-07-14 Thread GitBox 


rymurr commented on a change in pull request #7753:
URL: https://github.com/apache/arrow/pull/7753#discussion_r454387527



##
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:
   `arrowbuf.asNettyBuffer()` fails as we don't expose Netty dependencies 
in the Java API anymore. I have changed it to just take an `ArrowBuf`, I am not 
sure why it originally needed a Netty Buffer.





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] rymurr commented on a change in pull request #7753: ARROW-9385: [Python] finish Fix JPype tests

2020-07-14 Thread GitBox 


rymurr commented on a change in pull request #7753:
URL: https://github.com/apache/arrow/pull/7753#discussion_r454383698



##
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:
   Hey @pitrou I am not actually sure. The test passes locally for me with 
`base=arrowbuf`. I have uploaded a patch w/ this removed. Lets see if its just 
my machine.





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