Skip to content

ARROW-16673: [Java] C data interface: Allow ownership transferring for imported buffer #13248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zhztheplayer wants to merge 2 commits into from
Closed

ARROW-16673: [Java] C data interface: Allow ownership transferring for imported buffer #13248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4fa27b
Allow transfering ownsership for C Java ABI imported buffers
zhztheplayer May 17, 2022
a0a1ba1
style
zhztheplayer May 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion java/c/src/main/java/org/apache/arrow/c/CDataReferenceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member

@pitrou pitrou May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that targetAllocator isn't involved in this method at all?

Copy link
Member Author

@zhztheplayer zhztheplayer May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of. I would understand this as the original design of CDataReferenceManager not to bind itself to any allocator.

To me it would be great if we take some time to find if it is possible to make some refactors on ReferenceManager and BufferLedger to decouple allocator instance from the base interface ReferenceManager. Sounds OK to me that a native memory block doesn't "belong to" any Java side allocator.

Copy link
Contributor

@lwhite1 lwhite1 Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 that it should have this signature instead of something more descriptive.

Copy link

@adzcai adzcai Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you trying to make the CDataReferenceManager usable in code that supports other ReferenceManager implementations?

This would be helpful for a project I'm currently working on. For CDataRefereneceManager specifically, though, do you know if there's currently any way to copy a ValueVector? I'm essentially trying to clone a VectorSchemaRoot that's read from a Parquet file into a RootAllocator.

Copy link
Contributor

@lwhite1 lwhite1 Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adzcai Is the 'This' that would be helpful is the new method that doesn't increment the reference count, or some other aspect of the PR?

Copy link
Contributor

@emkornfield emkornfield Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like this might be a partial fix, that might have unintended consequences. It seems like this was already discussed on the main page of the PR was there any resolution here?

targetArrowBuf.readerIndex(sourceBuffer.readerIndex());
targetArrowBuf.writerIndex(sourceBuffer.writerIndex());

return new OwnershipTransferResult() {
Copy link
Contributor

@emkornfield emkornfield Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can OwnershipTransferNOOP be used here?

@Override
public boolean getAllocationFit() {
return true;
}

@Override
public ArrowBuf getTransferredBuffer() {
return targetArrowBuf;
}
};
}

@Override
Expand Down
32 changes: 32 additions & 0 deletions java/c/src/test/java/org/apache/arrow/c/RoundtripTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
import org.apache.arrow.vector.types.pojo.Field;
import org.apache.arrow.vector.types.pojo.FieldType;
import org.apache.arrow.vector.types.pojo.Schema;
import org.apache.arrow.vector.util.TransferPair;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -704,6 +705,37 @@ public void testImportReleasedArray() {
}
}

@Test
public void testTransferImportedBuffer() {
VectorSchemaRoot imported;

// Consumer allocates empty structures
try (ArrowSchema consumerArrowSchema = ArrowSchema.allocateNew(allocator);
ArrowArray consumerArrowArray = ArrowArray.allocateNew(allocator)) {
try (VectorSchemaRoot vsr = createTestVSR()) {
// Producer creates structures from existing memory pointers
try (ArrowSchema arrowSchema = ArrowSchema.wrap(consumerArrowSchema.memoryAddress());
ArrowArray arrowArray = ArrowArray.wrap(consumerArrowArray.memoryAddress())) {
// Producer exports vector into the C Data Interface structures
Data.exportVectorSchemaRoot(allocator, vsr, null, arrowArray, arrowSchema);
}
}
// Consumer imports vector
imported = Data.importVectorSchemaRoot(allocator, consumerArrowArray, consumerArrowSchema, null);
}

try (BufferAllocator targetAllocator = allocator.newChildAllocator("transfer allocator", 0, Long.MAX_VALUE)) {
for (FieldVector fieldVector : imported.getFieldVectors()) {
int cnt = fieldVector.getValueCount();
// Transfer buffer ownerships. Should not report any error
TransferPair transferPair = fieldVector.getTransferPair(targetAllocator);
transferPair.transfer();
ValueVector to = transferPair.getTo();
assertEquals(cnt, to.getValueCount());
Copy link
Member

@lidavidm lidavidm Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does targetAllocator report for allocated bytes after the transfer?

}
}
}

private VectorSchemaRoot createTestVSR() {
BitVector bitVector = new BitVector("boolean", allocator);

Expand Down