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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
