Add Builder style APIs and docs for `FlightData`, `FlightInfo`, `FlightEndpoint`, `Locaation` and `Ticket` #4294

alamb · 2023-05-27T12:03:45Z

Which issue does this PR close?

Rationale for this change

It is a PITA to create FlightInfo as described on #4281

As I was working on the code and examples, I realized it was a pain to create all the other types as well

What changes are included in this PR?

Add Builder style APIs and docs for FlightData, FlightInfo, FlightEndpoint, Locaation and Ticket

Since these are all prost generated stucts anyways all the fields are pub so I figured simply making it easier to modify them would be good

I changed the signatures of FlightInfo::new() and FlightData::new() as the existing functions are not very useful, which I will explain inline

…point, Locaation and Ticket

alamb · 2023-05-27T12:04:30Z

arrow-flight/examples/flight_sql_server.rs

@@ -222,26 +221,15 @@ impl FlightSqlService for FlightSqlServiceImpl {
            ticket: Some(ticket),
            location: vec![loc],
        };
-        let endpoints = vec![endpoint];
+        let info = FlightInfo::new()


This is a pretty good example of the "before" and "after" that this PR provides.

It doesn't do anything different, it is just easier to work with the structures

alamb · 2023-05-27T12:04:42Z

arrow-flight/examples/flight_sql_server.rs

+        let ticket = Ticket::new(query.encode_to_vec());
+        let endpoint = FlightEndpoint::new().with_ticket(ticket);
+
+        let flight_info = FlightInfo::new()


Same thing here -- I claim this is a lot easier to read

alamb · 2023-05-27T12:06:32Z

arrow-flight/src/lib.rs

+
+    /// Add a data body. See [`IpcDataGenerator`] to create this data.
+    ///
+    /// [`IpcDataGenerator`]: arrow_ipc::writer::IpcDataGenerator


Making a builder API gives a location to add some comments on what these fields should contain (the field description comes from the protobuf so is not Rust specific)

alamb · 2023-05-27T12:06:50Z

arrow-flight/src/lib.rs

-            app_metadata: app_metadata.into(),
-            data_body: data_body.into(),
-        }
+    pub fn new() -> Self {


This is technically a breaking change, but since all the fields on FlightData are pub anyways I don't think the old constructor was very useful (it was not used in the arrow codebase, for what that is worth)

alamb · 2023-05-27T12:07:06Z

arrow-flight/src/lib.rs

@@ -433,24 +464,45 @@ impl FlightDescriptor {
 }

 impl FlightInfo {
-    /// Create a new [`FlightInfo`] that describes the access
-    /// coordinates for retrieval of a dataset.
-    pub fn new(


This is also technically a breaking change, but since all the fields on FlightData are pub anyways I don't think the old constructor was very useful (as above)

alamb · 2023-05-27T12:07:41Z

arrow-flight/src/lib.rs

+    /// encodes it using the default IPC options.
+    ///
+    /// Returns an error if `schema` can not be encoded into IPC form.
+    pub fn with_schema(mut self, schema: &Schema) -> ArrowResult<Self> {


This was one of the nastiest APIs to deal with (figuring out the right incantation to encode the schema)

alamb · 2023-05-27T12:08:15Z

arrow-flight/tests/flight_sql_client_cli.rs

-            total_bytes: batch.get_array_memory_size() as i64,
-            ordered: false,
-        };
+        let info = FlightInfo::new()


Again, I think the new version is much more readable (it is still fine to use the old syntax)

alamb · 2023-05-27T12:09:42Z

arrow-flight/src/lib.rs

-        ordered: bool,
-    ) -> Self {
-        let IpcMessage(vals) = message;
+    /// Create a new, empty `FlightInfo`, describing where to fetch flight data


Rather than making a new FlightInfoBuilder I eventually decided to just add the builder style methods on FlightInfo itself:

It was simpler code

I wasn't entirely sure what was valid / invalid semantically, so extra error checking seemed unecessary

alamb · 2023-05-29T11:07:53Z

cc @avantgardnerio and @roeap -- I wonder if you have time to review this PR?

roeap

Happy to, and looking great! Not only the figuring out he first time part is much better now, but this may also avoid some fairly repetitive code in downstream implementations.

Just one minor comment, that you may also choose to ignore :).

roeap · 2023-05-29T11:12:53Z

arrow-flight/src/lib.rs

+    /// encodes it using the default IPC options.
+    ///
+    /// Returns an error if `schema` can not be encoded into IPC form.
+    pub fn with_schema(mut self, schema: &Schema) -> ArrowResult<Self> {


Just minor, but personally, I like to make the the fact that its fallible explicit in the name.

Suggested change

pub fn with_schema(mut self, schema: &Schema) -> ArrowResult<Self> {

pub fn try_with_schema(mut self, schema: &Schema) -> ArrowResult<Self> {

That is a good idea -- I will make that change

in a6c973f

avantgardnerio · 2023-05-29T16:05:48Z

arrow-flight/src/lib.rs

+            ordered: false,
+            // Flight says "Set these to -1 if unknown."
+            //
+            // https://github.com/apache/arrow-rs/blob/17ca4d51d0490f9c65f5adde144f677dbc8300e7/format/Flight.proto#L287-L289


Love this doc link, thank you for that

…uilder2

alamb · 2023-05-30T12:53:14Z

Thanks @roeap and @avantgardnerio

Add Builder style APIs and docs for FlightData, FlightInfo, FlightEnd…

d215863

…point, Locaation and Ticket

alamb added the api-change Changes to the arrow API label May 27, 2023

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels May 27, 2023

alamb commented May 27, 2023

View reviewed changes

alamb mentioned this pull request May 27, 2023

RFC: FlightInfoBuilder for helping create FlightInfo structures #4282

Closed

fix and clippy

f5c96e5

alamb marked this pull request as ready for review May 29, 2023 11:07

roeap approved these changes May 29, 2023

View reviewed changes

avantgardnerio approved these changes May 29, 2023

View reviewed changes

alamb added 2 commits May 30, 2023 08:27

Merge remote-tracking branch 'apache/master' into alamb/flight_info_b…

02e7796

…uilder2

Rename FlightInfo::with_schema to FlightInfo::try_with_schema

a6c973f

alamb merged commit 1b409a1 into apache:master May 30, 2023

alamb deleted the alamb/flight_info_builder2 branch May 30, 2023 12:53

alamb mentioned this pull request May 30, 2023

feat(flight): add helpers to handle CommandGetCatalogs, CommandGetSchemas, and CommandGetTables requests #4296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Builder style APIs and docs for `FlightData`, `FlightInfo`, `FlightEndpoint`, `Locaation` and `Ticket` #4294

Add Builder style APIs and docs for `FlightData`, `FlightInfo`, `FlightEndpoint`, `Locaation` and `Ticket` #4294

alamb commented May 27, 2023

alamb May 27, 2023

alamb May 27, 2023

alamb May 27, 2023

alamb May 27, 2023

alamb May 27, 2023

alamb May 27, 2023

alamb May 27, 2023

alamb May 27, 2023

alamb commented May 29, 2023

roeap left a comment

roeap May 29, 2023

alamb May 29, 2023

alamb May 30, 2023

avantgardnerio May 29, 2023

alamb commented May 30, 2023

	pub fn with_schema(mut self, schema: &Schema) -> ArrowResult<Self> {
	pub fn try_with_schema(mut self, schema: &Schema) -> ArrowResult<Self> {

Add Builder style APIs and docs for FlightData, FlightInfo, FlightEndpoint, Locaation and Ticket #4294

Add Builder style APIs and docs for FlightData, FlightInfo, FlightEndpoint, Locaation and Ticket #4294

Conversation

alamb commented May 27, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 29, 2023

roeap left a 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

alamb commented May 30, 2023

Add Builder style APIs and docs for `FlightData`, `FlightInfo`, `FlightEndpoint`, `Locaation` and `Ticket` #4294

Add Builder style APIs and docs for `FlightData`, `FlightInfo`, `FlightEndpoint`, `Locaation` and `Ticket` #4294