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/07/08 17:00:59 UTC
[GitHub] [arrow] lidavidm opened a new pull request #7685: ARROW-9362: [Java] Increment default MetadataVersion to V5
lidavidm opened a new pull request #7685:
URL: https://github.com/apache/arrow/pull/7685
This also enables Flight to write differing metadata versions.
Not implemented: any checks for unions in read/write based on metadata version.
I refactored TestFileWriter very heavily as I wanted to ensure IpcOptions was thoroughly tested and didn't want to duplicate code. I hope this doesn't make the change too hard to review.
----------------------------------------------------------------
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] lidavidm commented on pull request #7685: ARROW-9362: [Java] Support reading/writing V4 MetadataVersion
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#issuecomment-656949942
> So is this ok to merge before #7290 ?
This should be OK. It doesn't enable unions in the integration tests and prevents reading/writing unions with the old version. (Of course, someone could build master and start writing files with V5 metadata and the wrong union layout?)
----------------------------------------------------------------
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] pitrou commented on pull request #7685: ARROW-9362: [Java] Increment default MetadataVersion to V5
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#issuecomment-655663379
Does Java already implement the required IPC union layout and semantics? Otherwise perhaps we should defer this PR until the union work is done.
----------------------------------------------------------------
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] wesm closed pull request #7685: ARROW-9362: [Java] Support reading/writing V5 MetadataVersion
Posted by GitBox <gi...@apache.org>.
wesm closed pull request #7685:
URL: https://github.com/apache/arrow/pull/7685
----------------------------------------------------------------
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] BryanCutler commented on a change in pull request #7685: ARROW-9362: [Java] Support reading/writing V4 MetadataVersion
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#discussion_r452993057
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/ipc/MessageSerializerTest.java
##########
@@ -148,7 +150,7 @@ public void testSchemaDictionaryMessageSerialization() throws IOException {
public ExpectedException expectedEx = ExpectedException.none();
@Test
- public void testSerializeRecordBatch() throws IOException {
+ public void testSerializeRecordBatchV4() throws IOException {
Review comment:
doesn't this need an `IpcOption` to set `MetadataVersion.V4`?
----------------------------------------------------------------
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] BryanCutler commented on pull request #7685: ARROW-9362: [Java] Support reading/writing V4 MetadataVersion
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#issuecomment-656940054
So is this ok to merge before #7290 ?
----------------------------------------------------------------
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] lidavidm commented on pull request #7685: ARROW-9362: [Java] Increment default MetadataVersion to V5
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#issuecomment-655665159
We should wait for #7290, yes. (Is anyone reviewing it?)
Also, this now checks the metadata version against the schema before writing.
----------------------------------------------------------------
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] lidavidm commented on a change in pull request #7685: ARROW-9362: [Java] Support reading/writing V4 MetadataVersion
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#discussion_r453001215
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/ipc/MessageSerializerTest.java
##########
@@ -148,7 +150,7 @@ public void testSchemaDictionaryMessageSerialization() throws IOException {
public ExpectedException expectedEx = ExpectedException.none();
@Test
- public void testSerializeRecordBatch() throws IOException {
+ public void testSerializeRecordBatchV4() throws IOException {
Review comment:
Good catch, thank you. I've added the IpcOption.
----------------------------------------------------------------
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] lidavidm commented on pull request #7685: ARROW-9362: [Java] Support reading/writing V4 MetadataVersion
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#issuecomment-656738197
Would any Java maintainers be able to review? There's a bit of a circular dependency with this and #7290 - this is needed so that C++ will see the V5 metadata version and read with the right buffer layout, then #7290 will actually correct the buffer layout and enable the tests.
----------------------------------------------------------------
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 #7685: ARROW-9362: [Java] Increment default MetadataVersion to V5
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7685:
URL: https://github.com/apache/arrow/pull/7685#issuecomment-655642038
https://issues.apache.org/jira/browse/ARROW-9362
----------------------------------------------------------------
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