Repository navigation
GH-200: Keep registered type count when transferring a DenseUnionVector - #1381
sharanggupta wants to merge 2 commits into
Conversation
…onVector DenseUnionVector's transfer pair copied the type fields and type map into the target vector but left the target's nextTypeId at zero. The target therefore behaved as if no types were registered: transferring it again created no child transfer pairs and produced a vector whose values read back as null. Copy nextTypeId alongside the other type metadata when the transfer pairs are created, so both transfer() and splitAndTransfer() leave the target with the same registered types as the source. Closes apache#200.
|
Thank you for opening a pull request! Please label the PR with one or more of:
Also, add the 'breaking-change' label if appropriate. See CONTRIBUTING.md for details. |
|
The only failing check is "Ensure PR format", which needs a label. I don't have permission to add one; this is a |
There was a problem hiding this comment.
I think we should parameterize this test and test all types, ideally, instead of playing whack-a-mole. Or do other vectors have equivalent tests? (If so, I wonder if we could try and factor out common behavioral tests for vectors, instead of scattering them across different test classes)
There was a problem hiding this comment.
Good idea. Done in f41e9a8: I replaced the two DenseUnionVector-only tests with a parameterized test in TestSplitAndTransfer, where the transfer tests for the other vector kinds already live. For 18 kinds, it transfers a populated vector (or splits and transfers it), transfers the result again, and compares it with the original using VectorEqualsVisitor. The kinds are the fixed-width, variable-width and view vectors, fixed-size binary, the list, list-view and fixed-size list vectors, map, struct, union and dense union.
To answer the other question: without the fix, only the DenseUnionVector cases fail (expected [5, true, 7] but was [null, null, null]). The other 17 kinds already keep their values through a second transfer, so DenseUnionVector was the only one missing state there.
Factoring the other per-vector behavioural tests into shared parameterized tests looks worthwhile, but it touches many test classes, so I've kept it out of this PR. I can open an issue for it if that's useful.
Replace the two DenseUnionVector-only tests with a parameterized test in TestSplitAndTransfer. For 18 vector kinds it transfers, or splits and transfers, a populated vector, transfers the result again, and compares it with the original using VectorEqualsVisitor. Without the fix only the DenseUnionVector cases fail.
What's Changed
DenseUnionVector's transfer pair copies the type fields and type map into the target but leaves the target'snextTypeIdat 0. The target then acts as if no types are registered: transferring it again creates no child transfer pairs, and its values read back as null.This copies
nextTypeIdwhen the transfer pairs are created, so bothtransfer()andsplitAndTransfer()leave the target with the source's registered types.A parameterized test in
TestSplitAndTransfercovers 18 vector kinds. For each, it transfers a populated vector (or splits and transfers it), transfers the result again, and compares it with the original usingVectorEqualsVisitor. Without the fix, only theDenseUnionVectorcases fail (expected [5, true, 7] but was [null, null, null]).mvn -pl vector -am testwithTestSplitAndTransferandTestDenseUnionVectorpasses, as doesspotless:check.Closes #200.