-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-1278: [Integration] Adding integration tests for fixed_size_list #4309
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-1278: [Integration] Adding integration tests for fixed_size_list #4309
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4309 +/- ##
==========================================
+ Coverage 88.54% 88.97% +0.42%
==========================================
Files 796 704 -92
Lines 103206 94725 -8481
Branches 1253 0 -1253
==========================================
- Hits 91388 84277 -7111
+ Misses 11573 10448 -1125
+ Partials 245 0 -245
Continue to review full report at Codecov.
|
|
waiting for #4316 to be merged |
|
Is it expected for the integration tests to pass (there is support in Java for reading these?)? |
|
It seems like there is support: https://github.com/apache/arrow/blob/master/java/vector/src/test/java/org/apache/arrow/vector/ipc/TestArrowFile.java#L623 I'd need to spend more time figuring out exactly which java code is exercised by |
|
I just poked around the Java codebase. It seems it may work by accident, but it is not tested |
|
@bkietz what are the next steps here, do you need help with something on the Java side? |
e287bd8 to
ae45d3f
Compare
|
@emkornfield I have investigated the fixed size list tests. They're spare but present so I think we should merge this and add a JIRA to test fixed size list more thoroughly in Java. |
ae45d3f to
5806637
Compare
|
@bkietz sounds good, can you rebase? |
5806637 to
e7ed001
Compare
|
@emkornfield rebased and green except for a coredump in test_csv (I don't think that was me) |
| g++ \ | ||
| gcc \ | ||
| git \ | ||
| ninja-build \ |
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.
why this change?
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.
@emkornfield ninja is installed in ci/conda_env_cpp.yml so isn't this unused?
|
@bkietz one small question about a change to docker file. Otherwise LGTM |
|
@bkietz potentially not an expert on docker stuff, so I'd prefer cleanup as a separate PR |
|
@emkornfield reverted and green |
|
+1 LGTM, please file a JIRA for the follow-up work. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4309 +/- ##
==========================================
+ Coverage 88.54% 88.97% +0.42%
==========================================
Files 796 704 -92
Lines 103206 94725 -8481
Branches 1253 0 -1253
==========================================
- Hits 91388 84277 -7111
+ Misses 11573 10448 -1125
+ Partials 245 0 -245 ☔ View full report in Codecov by Sentry. |
Adds integration tests for fixed_size_list Also adds support for fixed_size_list to RecordBatchSerializer, which was omitted in apache#4278 Author: Benjamin Kietzman <bengilgit@gmail.com> Closes apache#4309 from bkietz/1278-integration-tests-for-fixed-size-list and squashes the following commits: 8b356f3 <Benjamin Kietzman> revert removal of ninja-build from dockerfile e7ed001 <Benjamin Kietzman> fix flake8 error 8ab4efc <Benjamin Kietzman> Adding integration tests for fixed_size_list
Adds integration tests for fixed_size_list
Also adds support for fixed_size_list to RecordBatchSerializer, which was omitted in #4278