ARROW-10962: [FlightRPC][Java] fill in empty body buffer if needed #8963
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Requesting review from @praveenbingo and @liyafan82 as they seem to have merged the last few Java PRs into the arrow repo. The background here is that we are working on flight integration tests in Rust (e.g. #9049) and we found an edge case in the flight protobuf integration. |
|
Sorry for my late response. Will take a look late today. |
liyafan82
reviewed
Jan 4, 2021
| @@ -317,6 +325,39 @@ private void test(BiConsumer<FlightClient, BufferAllocator> consumer) throws Exc | |||
| } | |||
| } | |||
|
|
|||
| /** ARROW-10939: accept FlightData messages generated by Protobuf (which can omit empty fields). */ | |||
liyafan82
reviewed
Jan 4, 2021
| @@ -317,6 +325,39 @@ private void test(BiConsumer<FlightClient, BufferAllocator> consumer) throws Exc | |||
| } | |||
| } | |||
|
|
|||
| /** ARROW-10939: accept FlightData messages generated by Protobuf (which can omit empty fields). */ | |||
| @Test | |||
| public void testProtobufCompat() throws Exception { | |||
There was a problem hiding this comment.
Expanded to 'compatibility'
liyafan82
reviewed
Jan 4, 2021
| final ArrowRecordBatch rb = | ||
| marshaller.parse(new ByteArrayInputStream(protobufData.toByteArray())).asRecordBatch(); | ||
| Assert.assertEquals(rb.computeBodyLength(), 0); | ||
| } |
There was a problem hiding this comment.
nit: do you think we also need a test case for which a schema has an empty body?
There was a problem hiding this comment.
Yup, I added a test for this as well.
liyafan82
reviewed
Jan 5, 2021
| .build(); | ||
| Assert.assertEquals(0, protobufData.getDataBody().size()); | ||
| // Should not throw | ||
| marshaller.parse(new ByteArrayInputStream(protobufData.toByteArray())).asSchema(); |
There was a problem hiding this comment.
Maybe we need to assign the schema to an object and verify that its body is null?
There was a problem hiding this comment.
The schema itself has no body. I've added a test to check the parsed message.
liyafan82
reviewed
Jan 5, 2021
| Flight.FlightDescriptor descriptor = FlightDescriptor.command(new byte[0]).toProtocol(); | ||
| try (final ArrowMessage message = new ArrowMessage(descriptor, schema, new IpcOption())) { | ||
| Assert.assertEquals(ArrowMessage.HeaderType.SCHEMA, message.getMessageType()); | ||
| try (final InputStream serialized = marshaller.stream(message)) { |
There was a problem hiding this comment.
We can extract a common method for the logic of writing the message to a ByteArrayOutputStream?
liyafan82
reviewed
Jan 5, 2021
| final MethodDescriptor.Marshaller<ArrowMessage> marshaller = ArrowMessage.createMarshaller(allocator); | ||
| final ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| Flight.FlightDescriptor descriptor = FlightDescriptor.command(new byte[0]).toProtocol(); | ||
| try (final ArrowMessage message = new ArrowMessage(descriptor, schema, new IpcOption())) { |
There was a problem hiding this comment.
Do we need to check the message body here?
There was a problem hiding this comment.
I've added assertions here.
liyafan82
approved these changes
Jan 6, 2021
|
#9049 is still blocked on getting this PR merged. Any chance you can do so @liyafan82 ? |
|
🎉 thanks @liyafan82 -- I didn't do anything, it was all @lidavidm . Thanks @lidavidm for the efforts! |
nevi-me
pushed a commit
that referenced
this pull request
Jan 9, 2021
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 🎉 The middleware scenario tests are currently skipped because they will fail until `tonic` can be updated to a version containing [a fix having to do with trailers](hyperium/tonic#510); this is tracked in [ARROW-10961](https://issues.apache.org/jira/browse/ARROW-10961). Some Rust <-> Java integration tests will fail until [this PR is merged](#8963); I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!! Closes #9049 from carols10cents/rust-flight-integration Lead-authored-by: Carol (Nichols || Goulding) <[email protected]> Co-authored-by: Jake Goulding <[email protected]> Signed-off-by: Neville Dipale <[email protected]>
alamb
pushed a commit
to apache/arrow-rs
that referenced
this pull request
Apr 20, 2021
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 🎉 The middleware scenario tests are currently skipped because they will fail until `tonic` can be updated to a version containing [a fix having to do with trailers](hyperium/tonic#510); this is tracked in [ARROW-10961](https://issues.apache.org/jira/browse/ARROW-10961). Some Rust <-> Java integration tests will fail until [this PR is merged](apache/arrow#8963); I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!! Closes #9049 from carols10cents/rust-flight-integration Lead-authored-by: Carol (Nichols || Goulding) <[email protected]> Co-authored-by: Jake Goulding <[email protected]> Signed-off-by: Neville Dipale <[email protected]>
GeorgeAp
pushed a commit
to sirensolutions/arrow
that referenced
this pull request
Jun 7, 2021
Fixes compatibility with standard Protobuf implementations, which may omit serialization of an empty field. In the case of an empty record batch, this would cause the Java client to fail a precondition check since there would be no body buffer instead of a single empty body buffer. Closes apache#8963 from lidavidm/arrow-10962 Authored-by: David Li <[email protected]> Signed-off-by: liyafan82 <[email protected]>
GeorgeAp
pushed a commit
to sirensolutions/arrow
that referenced
this pull request
Jun 7, 2021
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 🎉 The middleware scenario tests are currently skipped because they will fail until `tonic` can be updated to a version containing [a fix having to do with trailers](hyperium/tonic#510); this is tracked in [ARROW-10961](https://issues.apache.org/jira/browse/ARROW-10961). Some Rust <-> Java integration tests will fail until [this PR is merged](apache#8963); I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!! Closes apache#9049 from carols10cents/rust-flight-integration Lead-authored-by: Carol (Nichols || Goulding) <[email protected]> Co-authored-by: Jake Goulding <[email protected]> Signed-off-by: Neville Dipale <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes compatibility with standard Protobuf implementations, which may omit serialization of an empty field. In the case of an empty record batch, this would cause the Java client to fail a precondition check since there would be no body buffer instead of a single empty body buffer.