[GitHub] [arrow-rs] alamb commented on a diff in pull request #4359: feat(flight): add xdbc type info helpers

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #4359:
URL: https://github.com/apache/arrow-rs/pull/4359#discussion_r1218469383


##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -544,12 +580,11 @@ impl FlightSqlService for FlightSqlServiceImpl {
 
     async fn do_get_xdbc_type_info(
         &self,
-        _query: CommandGetXdbcTypeInfo,
+        query: CommandGetXdbcTypeInfo,
         _request: Request<Ticket>,
     ) -> Result<Response<<Self as FlightService>::DoGetStream>, Status> {
-        Err(Status::unimplemented(
-            "do_get_xdbc_type_info not implemented",
-        ))
+        let stream = INSTANCE_XDBC_INFO.encode(query).map_err(Status::from);

Review Comment:
   I may have missed it, but this doesn't seem to ever construct a `XdbcTypeInfoListBuilder` (instead it calls `encode` directly)
   
   What do you think about making this mirror the other builders, so the code would look like
   
   ```rust
           let mut builder = query.into_builder(&INSTANCE_XBDC_INFO);
           let schema = builder.schema();
           let batch = builder.build();
           let stream = FlightDataEncoderBuilder::new()
               .with_schema(schema)
               .build(futures::stream::once(async { batch }))
               .map_err(Status::from);
           Ok(Response::new(Box::pin(stream)))
    ```
   
   An alternate idea for construction 🤔 
   
   ```rust
           let mut builder = query.into_builder()
           builder.append_list(&INSTANCE_XBDC_INFO);?
   ```
   
   As you say, I agree we should also apply the same pattern (whatever it is) to `SqlInfoList`. I recommend we do so as part of another PR after we have worked out the pattern for `CommandGetXdbcTypeInfo`
   
   
   Finally, in terms of avoiding the boiler plate construction of 
   
   ```
           let batch = builder.build();
           let stream = FlightDataEncoderBuilder::new()
               .with_schema(schema)
               .build(futures::stream::once(async { batch }))
               .map_err(Status::from);
           Ok(Response::new(Box::pin(stream)))
   ```
   
   Maybe we can write some sort of generic function (either on `FlightDataEncoderBuilder` or as a free function) that takes a `impl Into<SchemaAndBatch>`
   
   where `SchemaAndBatch` looks something like
   
   ```rust
   pub trait SchemaAndBatch {
      /// produce the schema and result of encoding
     fn schema_and_batch(self) -> (SchemaRef, Result<RecordBatch>);
   }
   
   impl SchemaAndBatch for XdbcTypeInfoListBuilder {
   ...
   }
   ```
   
   Though maybe that is overly complicated 🤔 



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org