-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-11788: [Java] Fix appending empty delta vectors #9581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
siddharthteotia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| try (IntVector target = new IntVector("", allocator); | ||
| IntVector delta = new IntVector("", allocator)) { | ||
|
|
||
| target.allocateNew(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, as ValueVectorDataPopulator.setVector will do the allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all explicit calls for allocation if the utility method performs the allocation.
| assertEquals(10, target.getValueCount()); | ||
|
|
||
| try (IntVector expected = new IntVector("expected", allocator)) { | ||
| expected.allocateNew(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed.
| try (VarCharVector target = new VarCharVector("", allocator); | ||
| VarCharVector delta = new VarCharVector("", allocator)) { | ||
|
|
||
| target.allocateNew(5, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed either, as ValueVectorDataPopulator.setVector calls the getSafe method.
|
@nbruno Thanks for the PR. Merging. |
This PR fixes a bug where appending an empty list vector fails with a `NullPointerException`. Instead of attempting to process the delta vector, we just return early with the unmodified target vector, since there is nothing to append to the target vector from the delta vector. In addition, it fixes cases where appending an empty delta vector would've caused the same NPE, or where it'd be an optimization to not attempt processing an empty delta vector. Unit tests are added for all supported `VectorAppender` vector types. Closes apache#9581 from nbruno/ARROW-11788 Authored-by: Nick Bruno <[email protected]> Signed-off-by: liyafan82 <[email protected]>
This PR fixes a bug where appending an empty list vector fails with a
NullPointerException. Instead of attempting to process the delta vector, we just return early with the unmodified target vector, since there is nothing to append to the target vector from the delta vector. In addition, it fixes cases where appending an empty delta vector would've caused the same NPE, or where it'd be an optimization to not attempt processing an empty delta vector. Unit tests are added for all supportedVectorAppendervector types.