Skip to content
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

ARROW-10508 [Java] Allow FixedSizeListVector to have empty children #8605

Closed
wants to merge 4 commits into from

Conversation

Kopilov
Copy link
Contributor

@Kopilov Kopilov commented Nov 6, 2020

Hope this would be enough

@github-actions
Copy link

github-actions bot commented Nov 6, 2020

writeListVector(writer1, values1);
writeListVector(writer1, values2);
writeListVector(writer1, values3);
writer1.setValueCount(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a case to insert null into the vector?

@Kopilov
Copy link
Contributor Author

Kopilov commented Nov 12, 2020

@liyafan82 good idea, added test case to PR.
Also tested together with C++ in my demo project: commit, workflow.
Everything looks like OK

@liyafan82
Copy link
Contributor

@liyafan82 good idea, added test case to PR.
Also tested together with C++ in my demo project: commit, workflow.
Everything looks like OK

@Kopilov Thanks for your follow-up.

The new test case testVectorWithNulls is nice to have.
What I meant was that maybe we need a null slot in the list vector, and test our vector correctly distinguishes between an empty array and a null (sorry I did not made this clear enough)
I've prepared a commit for that. Could you please check?

@Kopilov
Copy link
Contributor Author

Kopilov commented Nov 15, 2020

@liyafan82 seems working correctly in all cases if we add writer.startList and writer.endList together with vector.setNull.
I have refactored writeListVector function in test to make it more universal.

May be we should add a getter for protected FixedSizeListVector vector prorerty in UnionFixedSizeListWriter and/or cache FixedSizeListVector.getWriter output (in current version new writer is created on every invocation). This allows writeListVector ans similar methods to have only vector or only writer parameter with keeping new functional.

Cross-test with C++ is again made in my project. But where are cross tests made in Apache Arrow itself?

@liyafan82
Copy link
Contributor

@liyafan82 seems working correctly in all cases if we add writer.startList and writer.endList together with vector.setNull.
I have refactored writeListVector function in test to make it more universal.

May be we should add a getter for protected FixedSizeListVector vector prorerty in UnionFixedSizeListWriter and/or cache FixedSizeListVector.getWriter output (in current version new writer is created on every invocation). This allows writeListVector ans similar methods to have only vector or only writer parameter with keeping new functional.

Cross-test with C++ is again made in my project. But where are cross tests made in Apache Arrow itself?

@Kopilov Thanks for your effort. It looks reasonable. Will merge soon if there are no more comments.

Concerning the problem of caching writers, maybe you can open a separate JIRA for it, if you think it necessary.

We have integeration tests across different languages in "Integration / AMD64 Conda Integration Test (pull_request)" when a PR is submitted.

@liyafan82
Copy link
Contributor

Merging. Thanks for your effort. @Kopilov

@liyafan82 liyafan82 closed this in dbdce71 Nov 19, 2020
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
Hope this would be enough

Closes apache#8605 from Kopilov/FixedSizeListVector

Lead-authored-by: Kopilov Aleksandr <kopilov.ad@gmail.com>
Co-authored-by: liyafan82 <fan_li_ya@foxmail.com>
Signed-off-by: liyafan82 <fan_li_ya@foxmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants