alamb commented on code in PR #4294:
URL: https://github.com/apache/arrow-rs/pull/4294#discussion_r1207916592
##########
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()
Review Comment:
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
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -292,32 +280,14 @@ impl FlightSqlService for FlightSqlServiceImpl {
request: Request<FlightDescriptor>,
) -> Result<Response<FlightInfo>, Status> {
let flight_descriptor = request.into_inner();
- let ticket = Ticket {
- ticket: query.encode_to_vec().into(),
- };
-
- let options = IpcWriteOptions::default();
-
- // encode the schema into the correct form
- let IpcMessage(schema) = SchemaAsIpc::new(SqlInfoList::schema(),
&options)
- .try_into()
- .expect("valid sql_info schema");
-
- let endpoint = vec![FlightEndpoint {
- ticket: Some(ticket),
- // we assume users wnating to use this helper would reasonably
- // never need to be distributed across multile endpoints?
- location: vec![],
- }];
-
- let flight_info = FlightInfo {
- schema,
- flight_descriptor: Some(flight_descriptor),
- endpoint,
- total_records: -1,
- total_bytes: -1,
- ordered: false,
- };
+ let ticket = Ticket::new(query.encode_to_vec());
+ let endpoint = FlightEndpoint::new().with_ticket(ticket);
+
+ let flight_info = FlightInfo::new()
Review Comment:
Same thing here -- I claim this is a lot easier to read
##########
arrow-flight/tests/flight_sql_client_cli.rs:
##########
@@ -167,42 +166,31 @@ impl FlightSqlService for FlightSqlServiceImpl {
let batch = Self::fake_result().unwrap();
- let IpcMessage(schema_bytes) =
- SchemaAsIpc::new(batch.schema().as_ref(),
&IpcWriteOptions::default())
- .try_into()
- .unwrap();
-
- let info = FlightInfo {
- schema: schema_bytes,
- flight_descriptor: None,
- endpoint: vec![
- FlightEndpoint {
- ticket: Some(Ticket {
- ticket: FetchResults {
- handle: String::from("part_1"),
- }
- .as_any()
- .encode_to_vec()
- .into(),
- }),
- location: vec![],
- },
- FlightEndpoint {
- ticket: Some(Ticket {
- ticket: FetchResults {
- handle: String::from("part_2"),
- }
- .as_any()
- .encode_to_vec()
- .into(),
- }),
- location: vec![],
- },
- ],
- total_records: batch.num_rows() as i64,
- total_bytes: batch.get_array_memory_size() as i64,
- ordered: false,
- };
+ let info = FlightInfo::new()
Review Comment:
Again, I think the new version is much more readable (it is still fine to
use the old syntax)
##########
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(
Review Comment:
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)
##########
arrow-flight/src/lib.rs:
##########
@@ -390,21 +391,51 @@ impl FlightData {
/// See [`FlightDataEncoderBuilder`] for a higher level API to
/// convert a stream of [`RecordBatch`]es to [`FlightData`]s
///
+ /// # Example:
+ ///
+ /// ```
+ /// # use bytes::Bytes;
+ /// # use arrow_flight::{FlightData, FlightDescriptor};
+ /// # fn encode_data() -> Bytes { Bytes::new() } // dummy data
+ /// // Get encoded Arrow IPC data:
+ /// let data_body: Bytes = encode_data();
+ /// // Create the FlightData message
+ /// let flight_data = FlightData::new()
+ /// .with_descriptor(FlightDescriptor::new_cmd("the command"))
+ /// .with_app_metadata("My apps metadata")
+ /// .with_data_body(data_body)
+ /// ```
+ ///
/// [`FlightDataEncoderBuilder`]: crate::encode::FlightDataEncoderBuilder
/// [`RecordBatch`]: arrow_array::RecordBatch
- pub fn new(
- flight_descriptor: Option<FlightDescriptor>,
- message: IpcMessage,
- app_metadata: impl Into<Bytes>,
- data_body: impl Into<Bytes>,
- ) -> Self {
- let IpcMessage(vals) = message;
- FlightData {
- flight_descriptor,
- data_header: vals,
- app_metadata: app_metadata.into(),
- data_body: data_body.into(),
- }
+ pub fn new() -> Self {
Review Comment:
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)
##########
arrow-flight/src/lib.rs:
##########
@@ -459,6 +511,51 @@ impl FlightInfo {
let msg = IpcMessage(self.schema);
msg.try_into()
}
+
+ /// Specify the schema for the response.
+ ///
+ /// Note this takes the arrow [`Schema`] (not the IPC schema) and
+ /// 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> {
Review Comment:
This was one of the nastiest APIs to deal with (figuring out the right
incantation to encode the schema)
##########
arrow-flight/src/lib.rs:
##########
@@ -390,21 +391,51 @@ impl FlightData {
/// See [`FlightDataEncoderBuilder`] for a higher level API to
/// convert a stream of [`RecordBatch`]es to [`FlightData`]s
///
+ /// # Example:
+ ///
+ /// ```
+ /// # use bytes::Bytes;
+ /// # use arrow_flight::{FlightData, FlightDescriptor};
+ /// # fn encode_data() -> Bytes { Bytes::new() } // dummy data
+ /// // Get encoded Arrow IPC data:
+ /// let data_body: Bytes = encode_data();
+ /// // Create the FlightData message
+ /// let flight_data = FlightData::new()
+ /// .with_descriptor(FlightDescriptor::new_cmd("the command"))
+ /// .with_app_metadata("My apps metadata")
+ /// .with_data_body(data_body)
+ /// ```
+ ///
/// [`FlightDataEncoderBuilder`]: crate::encode::FlightDataEncoderBuilder
/// [`RecordBatch`]: arrow_array::RecordBatch
- pub fn new(
- flight_descriptor: Option<FlightDescriptor>,
- message: IpcMessage,
- app_metadata: impl Into<Bytes>,
- data_body: impl Into<Bytes>,
- ) -> Self {
- let IpcMessage(vals) = message;
- FlightData {
- flight_descriptor,
- data_header: vals,
- app_metadata: app_metadata.into(),
- data_body: data_body.into(),
- }
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ /// Add a [`FlightDescriptor`] describing the data
+ pub fn with_descriptor(mut self, flight_descriptor: FlightDescriptor) ->
Self {
+ self.flight_descriptor = Some(flight_descriptor);
+ self
+ }
+
+ /// Add a data header
+ pub fn with_data_header(mut self, data_header: impl Into<Bytes>) -> Self {
+ self.data_header = data_header.into();
+ self
+ }
+
+ /// Add a data body. See [`IpcDataGenerator`] to create this data.
+ ///
+ /// [`IpcDataGenerator`]: arrow_ipc::writer::IpcDataGenerator
Review Comment:
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)
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
