ARROW-7073: [Java] Support concating vectors values in batch #5916
Conversation
There was a problem hiding this comment.
I think this method should be eliminated and a method should be added to the test data populators that can take literal values. It would make the tests easier to read and understand.
There was a problem hiding this comment.
Good suggestion. I have rewritten the test cases with the test data populators.
There was a problem hiding this comment.
see comment below on taking literal values instead of created values in loops. having a seperate method here make it harded to understand the actual test.
There was a problem hiding this comment.
I have revised the test cases accordingly. Thanks.
There was a problem hiding this comment.
don't use loops. Instead use or add methods to the population helper methods.
There was a problem hiding this comment.
Revised. Thank you.
There was a problem hiding this comment.
use helper methods to populate values.
There was a problem hiding this comment.
Good suggestion, populating vectors with literals make the code more readable.
There was a problem hiding this comment.
I think this class should probably not be public, instead there should be a public static method that uses this implementation and takes a variable length list of vectors to append. I think this would be much easier to understand. What do youthink?
There was a problem hiding this comment.
Sounds good to me. I will make it package private.
liyafan82
force-pushed
the
fly_1125_veccat
branch
from
6634d3f to
dd60040
Compare
There was a problem hiding this comment.
I thought I saw we have an assertVectorsEquals someplace in the code?
There was a problem hiding this comment.
Sorry I did not find one. So I have created one.
There was a problem hiding this comment.
use a ValueVectorDataPopulator here as well (maybe make the inputs above slightly shorter?)
There was a problem hiding this comment.
Revised. Please take a look. Thank you.
liyafan82
force-pushed
the
fly_1125_veccat
branch
from
41e0c44 to
3ba3c3d
Compare
There was a problem hiding this comment.
Thanks, I think this looks reasonable in general. One last question, do you think some null values should be added into the vectors to test proper appending of the bitmaps?
There was a problem hiding this comment.
Sure. I think adding null values should be beneficial.
I will revise the test later.
There was a problem hiding this comment.
Tests are revised to include null values. Fortunately, a bug was found.
|
+1 thanks @liyafan82 |
|
@liyafan82 could you rebase I just wanted to make sure visitor code isn't affected by DenseUnionVector |
@emkornfield Thanks a lot for your effort. BTW, today is the first day of the Chinese new year, a very special day for us. |
Enjoy your vacation and same to you. Happy new year! |
liyafan82
force-pushed
the
fly_1125_veccat
branch
from
3e36883 to
3bdfe02
Compare
@emkornfield I have rebased the PR. We need to support batch append for dense union vectors, and I am working on it. If we do it in another PR, may be we can finish this one now? |
|
I think another PR is fine for the dense vectors. |
Thanks a lot for your feedback. I will open an issue to track it. |
|
+1, thank you @liyafan82 |
|
@liyafan82 sorry I think another change was merged that conflicts with this, could you rebase? |
liyafan82
force-pushed
the
fly_1125_veccat
branch
from
3bdfe02 to
94b407c
Compare
@emkornfield Rebased. Please take a look. Thanks a lot for your effort. |
We need a way to copy vector values in batch. Currently, we have copyFrom and copyFromSafe APIs. However, they are not enough, as copying values individually is not performant. Closes #5916 from liyafan82/fly_1125_veccat and squashes the following commits: 94b407c <liyafan82> Support dense union vector ee49dc6 <liyafan82> Add tests with null values ad33e23 <liyafan82> Rewrite tests with vector populator for result verification c89211a <liyafan82> Rewrite tests with vector populator and provide static utility 7c13ede <liyafan82> Support concating vectors values in batch Authored-by: liyafan82 <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>
We need a way to copy vector values in batch. Currently, we have copyFrom and copyFromSafe APIs. However, they are not enough, as copying values individually is not performant. Closes apache#5916 from liyafan82/fly_1125_veccat and squashes the following commits: 94b407c <liyafan82> Support dense union vector ee49dc6 <liyafan82> Add tests with null values ad33e23 <liyafan82> Rewrite tests with vector populator for result verification c89211a <liyafan82> Rewrite tests with vector populator and provide static utility 7c13ede <liyafan82> Support concating vectors values in batch Authored-by: liyafan82 <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>
3 participants
We need a way to copy vector values in batch. Currently, we have copyFrom and copyFromSafe APIs. However, they are not enough, as copying values individually is not performant.