[GitHub] [arrow] lwhite1 commented on a diff in pull request #13248: ARROW-16673: [Java] C data interface: Allow ownership transferring for imported buffer

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

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


##########
java/c/src/main/java/org/apache/arrow/c/CDataReferenceManager.java:
##########
@@ -104,7 +104,21 @@ public ArrowBuf deriveBuffer(ArrowBuf sourceBuffer, long index, long length) {
 
   @Override
   public OwnershipTransferResult transferOwnership(ArrowBuf sourceBuffer, BufferAllocator targetAllocator) {
-    throw new UnsupportedOperationException();
+    ArrowBuf targetArrowBuf = this.deriveBuffer(sourceBuffer, 0, sourceBuffer.capacity());

Review Comment:
   It concerns me somewhat that the method doesn't perform its stated purpose: "Transfer the memory accounting ownership of this ArrowBuf to another allocator", but I also understand what you're saying about the referenceManager not binding itself to any allocator: the getAllocator() method in this class always returns null.  Even so, I wonder if it's not better to give this operation a name that matches the functionality better. 
   
   I noticed also that this implementation is very similar to  `public ArrowBuf retain(ArrowBuf srcBuffer, BufferAllocator targetAllocator)` which also has an unused bufferAllocator argument. The main difference between the two methods seems to be that the retain() implementation increments the reference count, while this method does not.  It also returns the newly derived buffer directly, rather than wrapped in a OwnershipTransferResult. But since the flag in the result here is always `true`, it doesn't seem to add too much. 
   
   Are you trying to make the CDataReferenceManager usable in code that supports other ReferenceManager implementations? If not, I guess I would just ask that you confirm that a variation without the reference count increment is needed, and if so, that it should have this signature instead of something more descriptive. 



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