Update flight definitions including backwards-incompatible change to GetSchema #2586

liukun4515 · 2022-08-25T07:31:55Z

Which issue does this PR close?

Closes #2571
Closes #2445

Rationale for this change

What changes are included in this PR?

Update the dir format from https://github.com/apache/arrow/tree/master/format

Are there any user-facing changes?

liukun4515 · 2022-08-25T07:34:24Z

I don't know why there are many format changes in the result of build file.
@carols10cents I find the format changed in this commit 5653512

I just build the arrow-rs by cargo build in my mac.

liukun4515 · 2022-08-25T07:35:36Z

arrow-flight/src/arrow.flight.protocol.rs

 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct HandshakeRequest {
    ///
-    ///  A defined protocol version
+    /// A defined protocol version


Why is there a formatting change here?

prost had a bug that got fixed. I don't think this is a huge problem. tokio-rs/prost#694

it's not a huge problem, but it will introduce useless changes in the git diff for each build

liukun4515 · 2022-08-25T07:43:45Z

Can we add ignore item in the .ignore file to ignore the changes for arrow.flight.protocol.rs and arrow.flight.protocol.sql.rs?
@alamb @tustvold

tustvold · 2022-08-25T07:54:03Z

Can we add ignore item in the .ignore file to ignore the changes for arrow.flight.protocol.rs and arrow.flight.protocol.sql.rs?

Are you suggesting we don't check in the generated code? I personally agree with this, but I know that others prefer checking it in

tustvold · 2022-08-25T07:59:16Z

format/Flight.proto

@@ -193,7 +193,10 @@ message Result {
 * Wrap the result of a getSchema call
 */
 message SchemaResult {
-  // schema of the dataset as described in Schema.fbs::Schema.
+  // The schema of the dataset in its IPC form:


Did they change the protocol without updating the message, or have we just always been doing this wrong?

From the issue, I don't find the historical implementation for this RPC protocal.
Maybe this comment can provide some info for you apache/arrow#13853 (comment)

tustvold · 2022-08-25T07:59:47Z

arrow-flight/src/lib.rs

+impl TryFrom<SchemaAsIpc<'_>> for SchemaResult {
+    type Error = ArrowError;
+
+    fn try_from(schema_ipc: SchemaAsIpc) -> ArrowResult<Self> {


I'm surprised we can change this without also needing to change the flight client?

In particular I would expect changes to

impl TryFrom<&SchemaResult> for Schema

The flight client just can get the bytes array from GetSchema RPC.

The server/client grpc can be generated automatically.

But we provide logic to convert the SchemaResult to a Schema which will now fail??

It's better to make consistent with original implementation for me.

My point is twofold:

We clearly are missing a test as the change you have made to the protocol should be failing a test somewhere

We need to update the logic to interpret the SchemaResult which does exist and following this PR will be incorrect

As an aside I do really wonder why arrow flight is using opaque byte objects, the whole point of using an API DSL is to avoid this but hey ho, this protocol is wild

helps tooling and people find the generated code

I think the tooling has issues when it is generated into target/ which I think is best practice. I'd advocate for leaving it where it is, but .gitignoring it, which should solve formatting without breaking IDEs.

Will that work when published to crates.io or used via a git revision?

If I understand the thread, it sounds like this has been resolved:

use a flag to determine how to write

check for the continuation token to differentiate between versions of the protocol and avoid a breaking change

update the protobuf <-> native conversion code

add a 4 round-trip tests: old -> new, new -> old, old -> old, new -> new

?

Will that work when published to crates.io or used via a git revision?

I think it will work for cargo publish but I think anyone depending on a git revision would require protoc to be installed. I have not verified these claims though.

https://doc.rust-lang.org/cargo/commands/cargo-publish.html

This command will create a distributable, compressed .crate file with the source code of the package in the current directory

liukun4515 · 2022-08-29T10:07:22Z

If there is no idea about how to resolve the compatibility for the rust client/server, I will close this issue and keep it as it is.

liukun4515 · 2022-08-30T13:04:32Z

Arrow community will add IT to cover the RPC method. https://issues.apache.org/jira/browse/ARROW-17568

liukun4515 · 2022-08-30T13:07:45Z

I will add the backwards compatibility tomorrow.

liukun4515 · 2022-08-31T02:45:06Z

Can we add ignore item in the .ignore file to ignore the changes for arrow.flight.protocol.rs and arrow.flight.protocol.sql.rs?

Are you suggesting we don't check in the generated code? I personally agree with this, but I know that others prefer checking it in

What's your opinions about this? @alamb @carols10cents

I just know java/go don't check in the diff about the generated file in file.

liukun4515 · 2022-08-31T03:09:44Z

arrow-flight/src/lib.rs

@@ -254,10 +254,17 @@ impl From<SchemaAsIpc<'_>> for FlightData {
    }
 }

-impl From<SchemaAsIpc<'_>> for SchemaResult {
-    fn from(schema_ipc: SchemaAsIpc) -> Self {
-        let IpcMessage(vals) = flight_schema_as_flatbuffer(schema_ipc.0, schema_ipc.1);


@tustvold
The original implementation has diff with the original implementation of java.
The version of java has the length as the prefix for this buffer, but the original rust does have the prefix length.

/** * Write the serialized Message metadata, prefixed by the length, to the output Channel. This * ensures that it aligns to an 8 byte boundary and will adjust the message length to include * any padding used for alignment. * * @param out Output Channel * @param messageLength Number of bytes in the message buffer, written as little Endian prefix * @param messageBuffer Message metadata buffer to be written, this does not include any * message body data which should be subsequently written to the Channel * @param option IPC write options * @return Number of bytes written * @throws IOException on error */ public static int writeMessageBuffer(WriteChannel out, int messageLength, ByteBuffer messageBuffer, IpcOption option) throws IOException { // if write the pre-0.15.0 encapsulated IPC message format consisting of a 4-byte prefix instead of 8 byte int prefixSize = option.write_legacy_ipc_format ? 4 : 8; // ensure that message aligns to 8 byte padding - prefix_size bytes, then message body if ((messageLength + prefixSize ) % 8 != 0) { messageLength += 8 - (messageLength + prefixSize) % 8; } if (!option.write_legacy_ipc_format) { out.writeIntLittleEndian(IPC_CONTINUATION_TOKEN); } out.writeIntLittleEndian(messageLength); out.write(messageBuffer); out.align(); // any bytes written are already captured by our size modification above return messageLength + prefixSize; }

I think the original implementation for the rust is not right without the buffer size for the schema flatbuffer.

I would agree the original Rust implementation is different, I'm not sure I would go so far as to say it is incorrect. There is no particular need to send the length, given the protobuf is already providing the message framing.

agree, the layout of the schemaResult is bytes array, which is flexible but easy to change.

liukun4515 · 2022-08-31T04:07:41Z

After the discussion about the RPC getschema method.
The original protocal of serialization for schemaresult is

buffer_length: 4bytes
buffer_of_schema: N bytes

The current protocal of serialization for schemaresult is

continuation_maker: 4bytes
buffer_length: 4bytes
buffer_of_schema: N byte

All other languages are follow above protocal
changed by the jira ticket: https://issues.apache.org/jira/browse/ARROW-6313

liukun4515 · 2022-09-01T00:02:01Z

I think we have many thought about the protoc and generated file, can we discuss it in this #2616

In this PR, how about we just focus on the implementation of SchemaResult and the compatibility？

@tustvold @alamb @avantgardnerio

I think it's time to review this, I have fixed the issue of the compatibility and make consistent with the implementation of java/c++/go

avantgardnerio

The test seems to cover it, so LGTM!

tustvold

I think it is worth highlighting that the new client will be unable to communicate with old rust servers, I don't think there is a away around this but we should highlight this very clearly

tustvold · 2022-09-01T14:40:23Z

arrow-flight/src/lib.rs

+    let encoded_data = flight_schema_as_encoded_data(pair.0, pair.1);
+
+    let mut schema = vec![];
+    arrow::ipc::writer::write_message(&mut schema, encoded_data, pair.1)?;


I've double-checked this will write the continuation token 👍

arrow/src/ipc/convert.rs

tustvold · 2022-09-01T14:46:50Z

arrow-flight/src/lib.rs

@@ -254,10 +254,17 @@ impl From<SchemaAsIpc<'_>> for FlightData {
    }
 }

-impl From<SchemaAsIpc<'_>> for SchemaResult {
-    fn from(schema_ipc: SchemaAsIpc) -> Self {
-        let IpcMessage(vals) = flight_schema_as_flatbuffer(schema_ipc.0, schema_ipc.1);


I would agree the original Rust implementation is different, I'm not sure I would go so far as to say it is incorrect. There is no particular need to send the length, given the protobuf is already providing the message framing.

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

alamb · 2022-09-02T11:05:51Z

I would like to propose we hold off on merging this PR until I create an arrow-flight 22.0.0 release candidate (in a few hours time) -- Then we can merge this PR for release in arrow-flight 23.0.0 and start working on potential downstream integrations (such as ballista, iox, etc) that will change.

Thank you @liukun4515 and @tustvold for your work

avantgardnerio · 2022-09-02T15:19:16Z

hold off on merging this PR until I create an arrow-flight 22.0.0

+1

liukun4515 · 2022-09-03T09:06:13Z

I would like to propose we hold off on merging this PR until I create an arrow-flight 22.0.0 release candidate (in a few hours time) -- Then we can merge this PR for release in arrow-flight 23.0.0 and start working on potential downstream integrations (such as ballista, iox, etc) that will change.

@alamb If you have done the release for 22.0.0, please merge it.

ursabot · 2022-09-03T11:51:47Z

Benchmark runs are scheduled for baseline = e5b9d05 and contender = 4c1bb00. 4c1bb00 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

update flight doc and code

bbf77ac

liukun4515 requested review from tustvold and wesm August 25, 2022 07:32

liukun4515 commented Aug 25, 2022

View reviewed changes

github-actions bot added the arrow-flight Changes to the arrow-flight crate label Aug 25, 2022

liukun4515 changed the title ~~update flight doc and code~~ update flight doc and change the result of SchemaResult Aug 25, 2022

tustvold added the api-change Changes to the arrow API label Aug 25, 2022

tustvold reviewed Aug 25, 2022

View reviewed changes

fix cliyyp

05b7a2f

liukun4515 mentioned this pull request Aug 29, 2022

[Go][Rust] Deserializing a Byte array to a schema apache/arrow#13853

Closed

liukun4515 commented Aug 31, 2022

View reviewed changes

liukun4515 mentioned this pull request Aug 31, 2022

Do not check in the generated file from proto #2616

Closed

backward compatibility for schema result

f92b3b7

github-actions bot added the arrow Changes to the arrow crate label Aug 31, 2022

Merge remote-tracking branch 'upstream/master' into update_flight_#2571

eacb730

liukun4515 force-pushed the update_flight_#2571 branch from 21a966e to 0c72501 Compare August 31, 2022 08:02

avantgardnerio approved these changes Sep 1, 2022

View reviewed changes

fix lint

137e2c1

liukun4515 force-pushed the update_flight_#2571 branch from 0c72501 to 137e2c1 Compare September 1, 2022 03:13

tustvold approved these changes Sep 1, 2022

View reviewed changes

tustvold changed the title ~~update flight doc and change the result of SchemaResult~~ Update flight definitions including backwards-incompatible change to GetSchema Sep 1, 2022

liukun4515 and others added 2 commits September 2, 2022 16:19

Update arrow/src/ipc/convert.rs

ad0b77e

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

Update arrow/src/ipc/convert.rs

961c689

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

alamb merged commit 4c1bb00 into apache:master Sep 3, 2022

liukun4515 deleted the update_flight_#2571 branch September 5, 2022 00:56

This was referenced Sep 16, 2022

update the flight.proto and fix schema to SchemaResult #2571

Closed

SchemaResult in IPC deviates from other implementations #2445

Closed

Update flight definitions including backwards-incompatible change to GetSchema #2586

Update flight definitions including backwards-incompatible change to GetSchema #2586

Conversation

liukun4515 commented Aug 25, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

liukun4515 commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liukun4515 commented Aug 25, 2022 • edited Loading

tustvold commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liukun4515 Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

tustvold Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liukun4515 commented Aug 29, 2022 • edited Loading

liukun4515 commented Aug 30, 2022

liukun4515 commented Aug 30, 2022

liukun4515 commented Aug 31, 2022

liukun4515 Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liukun4515 commented Aug 31, 2022 • edited Loading

liukun4515 commented Sep 1, 2022

avantgardnerio left a comment

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Sep 2, 2022

avantgardnerio commented Sep 2, 2022

liukun4515 commented Sep 3, 2022

ursabot commented Sep 3, 2022

liukun4515 commented Aug 25, 2022 •

edited

Loading

liukun4515 commented Aug 25, 2022 •

edited

Loading

tustvold Aug 25, 2022 •

edited

Loading

liukun4515 Aug 25, 2022 •

edited

Loading

tustvold Aug 25, 2022 •

edited

Loading

liukun4515 commented Aug 29, 2022 •

edited

Loading

liukun4515 Aug 31, 2022 •

edited

Loading

liukun4515 commented Aug 31, 2022 •

edited

Loading