-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-1237: [JAVA] expose the ability to set lastSet #868
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
|
Is it possible to add a unit test for this? Can you also indicate in the PR title that this is a Java patch, e.g. |
|
Edited the PR title to include [JAVA]. |
DremioQA
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.
Let's add some unit tests for each version that has last set. Basically, set some data via memory access then update last set, then confirm a correct vector output.
| } | ||
|
|
||
| public void setLastSet(int value) { | ||
| <#if type.major = "VarLen">lastSet = value;</#if> |
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.
Is there a lastset for the list vector as well?
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.
Writing the unit tests.
Yes, the ListVector also has lastSet. Will add the API for ListVector as well and unit tests for each vector type; NullableVarCharVector, NullableVarBinaryVector, ListVector.
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.
|
@jacques-n There is a slightly orthogonal test case test based on VectorLoader and VectorUnloader. |
| final NullableVarCharVector.Accessor accessor1 = vector1.getAccessor(); | ||
| assertArrayEquals(STR1, accessor1.get(0)); | ||
| assertArrayEquals(STR2, accessor1.get(1)); | ||
| assertArrayEquals(STR3, accessor1.get(2)); |
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.
Given the main issue of holes/blanks in the items set, I suggest you modify these tests to set the valuecount to something higher (e.g. 15). Then verify that everything looks good.
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.
Added additional test and slightly modified existing ones.
The changes here expose the ability to set "lastSet" on Nullable<var length>Vector. I believe this is needed only for NullableVarCharVector and NullableVarBinaryVector. Hence the API is exposed through NullableValueVectors.java Author: siddharth <[email protected]> Closes apache#868 from siddharthteotia/ARROW-1237 and squashes the following commits: 786dfea [siddharth] ARROW-1237: addressed review comments and added more tests 73b2fc5 [siddharth] ARROW-1237: added some unit tests f8c7277 [siddharth] ARROW-1237: expose the ability to set lastSet
The changes here expose the ability to set "lastSet" on NullableVector.
I believe this is needed only for NullableVarCharVector and NullableVarBinaryVector. Hence the API is exposed through NullableValueVectors.java