Managing ownership in VectorSchemaRoot#addVector, recent changes miss the main fault.

[John-W-Lewis]

A recent change in VectorSchemaRoot#addVector provided a simple improvement, but completely misses a major shortcoming of this routine and, almost certainly, others like it. Based on the interest in that issue (#301), this issue might be of interest to @axreldable and @jbonofre.

One cannot change the FieldVectors belonging to VectorSchemaRoots by simply rearranging the elements of their Lists without considering the ownership of their underlying buffers. This can cause various problems, particularly when VectorSchemaRoots are closed. This is why TransferPair exists.

I would probably need some help with improving this as I'm quite new to using Apache Arrow (and very new to using GitHub). But I have written a possibly improved implementation if this would be of interest and would be open to collaborating on rectifying this bug.

[axreldable]

Hi @John-W-Lewis , thanks for raising this!

I’m also quite new to Arrow, so it would really help (at least for me) if you could clarify the bug report by explicitly outlining the expected vs. observed behavior (e.g. in #1117 ). Ideally, with code snippets of how to reproduce the bug (e.g. in #1129 ).

Also, if the negative impact of the change #1013 turns out to be significant and fixing it requires a non-trivial amount of work, let's consider rolling back the change and reopening the original issue #301 .

@John-W-Lewis , If you already have a potential fix which requires some improvements, consider to open a PR with your changes so people could suggest improvements there.

[John-W-Lewis]

Thank you, @axreldable, for responding!

Yes, I could have provided more details on how it does and doesn't behave. But we can understand the problem more easily at a higher, modelling, level.
The model is that a Vector owns multiple buffers which are deallocated when the Vector is closed. A VectorSchemaRoot owns multiple Vectors and when it is closed, it closes its Vectors, causing their buffers to be deallocated.
If we manipulate those relationships, in contravention of that model, so that one VectorSchemaRoot ends up "thinking" that it owns the buffers of Vectors being used by another VectorSchemaRoot, then one can close and cause the deallocation of buffers which are stlll being used by the other. It's simply not conforming to the intended model.

The TransferPair exists to manage the transfer of values between a pair of Vectors. I think that we can understand that the current implementation of addVector, probably removeVector, and possibly other methods is simply wrong without going into the sordid details of the scenarios in which we can get away with it or it can go wrong for each of, on my count, 45 classes for which TransferPair is implemented!

Yes, I have a potential fix, however it's very unlikely that I've found all of the places in which this fault exists, and I'm very new to using GitHub. Nevertheless, let me see what I can do, provided it's not expected to be the finished result!

[John-W-Lewis]

Well, @axreldable, I've opened #1145 with a fix for this.

As a newbie, I hope I've done it correctly ... although I admit to having a lot of AI assistance.

[John-W-Lewis]

Following up on PR #1145: while that fix correctly addresses the safety issue (by using TransferPair.transfer()), I've since realised it changes the semantics -- the source root is emptied after the operation, whereas the original (unsafe) implementation allowed both source and result to read the same data.

I've developed a better approach using a new VectorOps utility that provides a shareCopy operation: this creates a new vector sharing the same underlying allocations via reference counting. Both source and result remain fully usable, and the memory is only released when all sharing vectors/roots have been closed.

This gives us the best of both worlds: safe (proper reference counting) and preserves the original intended semantics.

I'll be opening a new PR with this approach and closing #1145 in its favour.