You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/11/06 14:22:35 UTC
[GitHub] [arrow] Kopilov opened a new pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Kopilov opened a new pull request #8605:
URL: https://github.com/apache/arrow/pull/8605
Hope this would be enough
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Kopilov commented on pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
Kopilov commented on pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#issuecomment-727626118
@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 `protected FixedSizeListVector vector` in `UnionFixedSizeListWriter` getter 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](https://github.com/Kopilov/Arrow_FixedSizeListVector_ZeroWidth/runs/1403388868). But where are cross tests made in Apache Arrow itself?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#discussion_r520405424
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestFixedSizeListVector.java
##########
@@ -402,6 +402,34 @@ public void testSplitAndTransfer() throws Exception {
}
}
+ @Test
+ public void testZeroWidthVector() throws Exception {
+ try (final FixedSizeListVector vector1 = FixedSizeListVector.empty("vector", 0, allocator)) {
+
+ UnionFixedSizeListWriter writer1 = vector1.getWriter();
+ writer1.allocate();
+
+ int[] values1 = new int[] {};
+ int[] values2 = new int[] {};
+ int[] values3 = new int[] {};
+
+ //set some values
+ writeListVector(writer1, values1);
+ writeListVector(writer1, values2);
+ writeListVector(writer1, values3);
+ writer1.setValueCount(3);
Review comment:
Maybe we can add a case to insert null into the vector?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 closed pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
liyafan82 closed pull request #8605:
URL: https://github.com/apache/arrow/pull/8605
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Kopilov edited a comment on pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
Kopilov edited a comment on pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#issuecomment-727626118
@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](https://github.com/Kopilov/Arrow_FixedSizeListVector_ZeroWidth/runs/1403388868). But where are cross tests made in Apache Arrow itself?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#issuecomment-726470885
> @liyafan82 good idea, added test case to PR.
> Also tested together with C++ in my demo project: [commit](https://github.com/Kopilov/Arrow_FixedSizeListVector_ZeroWidth/commit/3c2935382c6eed891844f7c4100b4bf5d7037646), [workflow](https://github.com/Kopilov/Arrow_FixedSizeListVector_ZeroWidth/runs/1391351598).
> 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#issuecomment-723111282
https://issues.apache.org/jira/browse/ARROW-10508
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#issuecomment-730210605
Merging. Thanks for your effort. @Kopilov
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Kopilov commented on pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
Kopilov commented on pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#issuecomment-726169585
@liyafan82 good idea, added test case to PR.
Also tested together with C++ in my demo project: [commit](https://github.com/Kopilov/Arrow_FixedSizeListVector_ZeroWidth/commit/3c2935382c6eed891844f7c4100b4bf5d7037646), [workflow](https://github.com/Kopilov/Arrow_FixedSizeListVector_ZeroWidth/runs/1391351598).
Everything looks like OK
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #8605: ARROW-10508 [Java] Allow FixedSizeListVector to have empty children
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #8605:
URL: https://github.com/apache/arrow/pull/8605#issuecomment-729338224
> @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](https://github.com/Kopilov/Arrow_FixedSizeListVector_ZeroWidth/runs/1403388868). 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org