GH-1142: Fix buffer ownership in VectorSchemaRoot addVector/removeVector

[John-W-Lewis]

What's Changed

VectorSchemaRoot#addVector and #removeVector previously created a new VectorSchemaRoot by sharing raw FieldVector references between the original and new root. This violates Arrow's ownership model: closing either root would release buffers still in use by the other, leading to use-after-free errors.

This fix uses TransferPair to properly transfer buffer ownership to the returned root, consistent with how VectorSchemaRoot#slice already handles this. After the operation, the original root's vectors are left in a transferred (empty) state and can be reused via allocateNew().

Tests added to verify that the returned root's data remains valid after the original root and input vector are closed.

Closes #1142.

Made with Cursor

[github-actions]

Thank you for opening a pull request!

Please label the PR with one or more of:

• bug-fix
• chore
• dependencies
• documentation
• enhancement

Also, add the 'breaking-change' label if appropriate.

See CONTRIBUTING.md for details.

[John-W-Lewis]

Being neither a chicken nor an egg, I understand that I am unable to label this as "bug-fix".
Thank you to whoever can do that.

[John-W-Lewis]

There are also a couple of enhancements I would suggest for VectorSchemaRoot but, based on the advice I've seen, it seems better to describe those in a separate PR.

[axreldable]

Hi @John-W-Lewis , thank you for the bug-fix. Yes, we need to wait for the maintainers to add the label and trigger the other checks. In the meantime, you can build locally, for example using Maven. Now, there is a format violation in TestVectorSchemaRoot.java.

mvn clean install
...
[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:3.4.0:check (spotless-check) on project arrow-vector: The following files had format violations:
[ERROR]     src/test/java/org/apache/arrow/vector/TestVectorSchemaRoot.java
[ERROR]         @@ -363,8 +363,7 @@
[ERROR]          ······intVector2.setValueCount(5);
[ERROR]          ······intVector3.setValueCount(5);
[ERROR]          
[ERROR]         -······VectorSchemaRoot·original·=
[ERROR]         -··········new·VectorSchemaRoot(Arrays.asList(intVector1,·intVector2));
[ERROR]         +······VectorSchemaRoot·original·=·new·VectorSchemaRoot(Arrays.asList(intVector1,·intVector2));
[ERROR]          ······original.setRowCount(5);
[ERROR]          
[ERROR]          ······VectorSchemaRoot·result·=·original.addVector(1,·intVector3);
[ERROR] Run 'mvn spotless:apply' to fix these violations.

You can fix it with the suggested command. mvn spotless:apply

With the formatting fix, the build was successful for me, which means that the unit tests passed.

[axreldable]

Other than the nit (nitpick) and the doc clarification, LGTM.

[axreldable]

nit: Consider extracting the transferring logic into a separate method to avoid code duplication.
e.g.

private static FieldVector transferVector(FieldVector vector) {
    TransferPair transferPair = vector.getTransferPair(vector.getAllocator());
    transferPair.transfer();
    return (FieldVector) transferPair.getTo();
  }

[John-W-Lewis]

Good suggestion., done.

[axreldable]

The removed vector's data is not transferred and is released.

is released is not 100% accurate and can be confusing. The vector remains in the original schema root until it's closed/cleared.

[John-W-Lewis]

Thank you for spotting this. You're right that "is released" was inaccurate.

Looking more closely, it was also a code issue. For the source vectors which are not removed, transfer() creates new ArrowBufs in the corresponding target vector, each referencing the same underlying allocation as its source counterpart, while the source vector's ArrowBufs release their references. So, after the operation, those source vectors' ArrowBufs no longer own any allocated memory. But for the removed vector, we weren't doing anything -- its ArrowBufs still held references to their underlying allocations, which would only be freed when the original root was eventually closed.

I've added a clear() on the removed vector so that its buffers also release their underlying allocations, making it consistent with the other vectors in the source root after the operation.

[John-W-Lewis]

@axreldable Thank you. TestVectorSchemaRoot.java formatting fixed.

[John-W-Lewis]

Closing in favour of #1149, which provides a better fix using a new VectorOps.shareCopy utility. That approach preserves the original intended semantics (both source and result roots remain readable) while making it safe through proper reference counting, rather than changing the semantics to transfer/empty the source.

Thank you @axreldable for your helpful review feedback on this PR -- it contributed to the thinking that led to the improved solution.