[GitHub] [arrow] lwhite1 commented on a diff in pull request #13249: ARROW-16674: [Java] C data interface: Reading as nioBuffer from imported buffer causes error

[GitBox <gi...@apache.org>]

lwhite1 commented on code in PR #13249:
URL: https://github.com/apache/arrow/pull/13249#discussion_r886909925


##########
java/c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -143,7 +143,9 @@ private List<ArrowBuf> importBuffers(ArrowArray.Snapshot snapshot) {
       if (bufferPtr != NULL) {
         // TODO(roee88): an API for getting the size for each buffer is not yet
         // available
-        buffer = new ArrowBuf(referenceManager, null, Integer.MAX_VALUE, bufferPtr);
+        int length = Integer.MAX_VALUE;

Review Comment:
   The length field in ArrowBuf is referred to in the ArrowBuf code as _capacity_, which seems to me to be clearer. It might be worthwhile to change the field name and the references (e.g. the argument name in the constructor), as well as the usage here. 



##########
java/c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -143,7 +143,9 @@ private List<ArrowBuf> importBuffers(ArrowArray.Snapshot snapshot) {
       if (bufferPtr != NULL) {
         // TODO(roee88): an API for getting the size for each buffer is not yet
         // available

Review Comment:
   This TODO should really be a JIRA ticket for the change. The rest of the comment (minus "TODO") could be left to explain why you have to set the writerIndex explicitly in the following code.



##########
java/c/src/test/java/org/apache/arrow/c/RoundtripTest.java:
##########
@@ -678,6 +679,22 @@ public void testSchema() {
     }
   }
 
+  @Test
+  public void testImportedBufferAsNioBuffer() {
+    IntVector imported;
+    try (final IntVector vector = new IntVector("v", allocator)) {
+      setVector(vector, 1, 2, 3, null);
+      imported = (IntVector) vectorRoundtrip(vector);
+      ArrowBuf dataBuffer = imported.getDataBuffer();
+      ByteBuffer nioBuffer = dataBuffer.nioBuffer().asReadOnlyBuffer();
+      nioBuffer.order(ByteOrder.nativeOrder());
+      assertEquals(1, nioBuffer.getInt(0));
+      assertEquals(2, nioBuffer.getInt(1 << 2));
+      assertEquals(3, nioBuffer.getInt(2 << 2));
+    }
+    imported.close();

Review Comment:
   @pitrou I believe it is, as _imported_ is a new FieldVector created in vectorRoundTrip and not a reference to the _vector_ created in the try-with-resources. 



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org