Repository navigation
Managing ownership in VectorSchemaRoot#addVector, recent changes miss the main fault. #1142
Description
Activity
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.
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 aVectorowns multiple buffers which are deallocated when the Vector is closed. AVectorSchemaRootowns multipleVectors and when it is closed, it closes itsVectors, causing their buffers to be deallocated.
If we manipulate those relationships, in contravention of that model, so that oneVectorSchemaRootends up "thinking" that it owns the buffers ofVectors being used by anotherVectorSchemaRoot, 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
TransferPairexists to manage the transfer of values between a pair of Vectors. I think that we can understand that the current implementation ofaddVector, probablyremoveVector, 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 whichTransferPairis 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!
Reacted by Aleksei StarikovWell, @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.
Reacted by Aleksei StarikovFollowing 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
VectorOpsutility that provides ashareCopyoperation: 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.
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 toVectorSchemaRoots by simply rearranging the elements of theirLists without considering the ownership of their underlying buffers. This can cause various problems, particularly whenVectorSchemaRoots are closed. This is whyTransferPairexists.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.