GH-79: Move splitAndTransferValidityBuffer to BaseValueVector - #777
Conversation
Sorry, something went wrong.
This comment has been minimized.
| * @param target target vector | ||
| */ | ||
| protected void splitAndTransferValidityBuffer( | ||
| int startIndex, int length, BaseValueVector target) { |
There was a problem hiding this comment.
Accepting BaseValueVector and not ValueVector here to keep the API (int, int, vector). If I accept ValueVector, then there there needs to be some lambda wrangling to pass in the vector-specific allocateValidityBuffer somehow, or that function needs to be added to ValueVector, both of which don't seem that great. From what I can tell everything except StructVector seems to be descend from BaseValueVector so chose this as part of the API.
Sorry, something went wrong.
There was a problem hiding this comment.
Checked that UnionVector doesn't extend it either (which is good - as it doesn't have a validity buffer)
Sorry, something went wrong.
| * @param length number of elements to be copied | ||
| * @param target target vector | ||
| */ | ||
| protected void sliceAndTransferValidityBuffer( |
There was a problem hiding this comment.
This implementation is the one being used by the vectors inside the complex/ directory. BaseValueVector#validityBuffer is protected, so overridden implementations there can't call target.validityBuffer to set a new value. Vectors in the top level org/apache/arrow/vector directory can add their overridden implementations. If at some point some other vector in complex/ wants to have a different version of this function, then I am afraid that vector will need to override splitAndTransferValidityBuffer entirely. Presumably don't want to expose a public setValidityBuffer on BaseValueVector.
Sorry, something went wrong.
lidavidm
left a comment
There was a problem hiding this comment.
Thanks!
Sorry, something went wrong.
| * @param target target vector | ||
| */ | ||
| protected void splitAndTransferValidityBuffer( | ||
| int startIndex, int length, BaseValueVector target) { |
There was a problem hiding this comment.
Checked that UnionVector doesn't extend it either (which is good - as it doesn't have a validity buffer)
Sorry, something went wrong.
| * | ||
| * @param byteSizeTarget desired size of the buffer | ||
| */ | ||
| protected void allocateValidityBuffer(final long byteSizeTarget) { |
There was a problem hiding this comment.
This is an abstract class - mark this abstract?
Sorry, something went wrong.
| final ArrowBuf slicedValidityBuffer = validityBuffer.slice(firstByteSource, byteSizeTarget); | ||
| target.validityBuffer = transferBuffer(slicedValidityBuffer, target.allocator); |
There was a problem hiding this comment.
Hmm, this only slightly differs from the 'base' implementation. However I think this one is more correct...I didn't dig too deep but it's possible the 'simple' one that just retains the buffer was a later/incorrect reimplementation or because they were all copy-pasted at some point only some types were adjusted to use transferBuffer
Sorry, something went wrong.
| */ | ||
| private void allocateValidityBuffer(final int validityBufferSize) { | ||
| @Override | ||
| protected void allocateValidityBuffer(final long validityBufferSize) { |
There was a problem hiding this comment.
Given many of these are very similar maybe they should delegate to a base implementation with super.allocateValidityBuffer then call any vector-specific logic at the end?
Sorry, something went wrong.
There was a problem hiding this comment.
Done, there is a slight behavior change though -- the validity buffer in BaseFixedWidthVector was not zeroing out its elements post allocation, it is now. Should be fine though I am thinking.
Sorry, something went wrong.
|
Resolved all conflicts. |
Sorry, something went wrong.
What's Changed
Move splitAndTransferValidityBuffer up to BaseValueVector.
This PR is not touching the implementation of this function in StructVector -- that is not being derived from BaseValueVector so some amount of duplication is probably fine.
Closes #79