alamb commented on code in PR #5433:
URL: https://github.com/apache/arrow-rs/pull/5433#discussion_r1510210520
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -617,7 +618,7 @@ impl FlightSqlService for FlightSqlServiceImpl {
&self,
_query: CommandPreparedStatementQuery,
_request: Request<PeekableFlightDataStream>,
- ) -> Result<Response<<Self as FlightService>::DoPutStream>, Status> {
+ ) -> Result<Option<DoPutPreparedStatementResult>, Status> {
Review Comment:
Isn't it the field on `DoPutPreparedStatementResult` that is optional? It
seems like this API would be cleaner and follow the rest of the API if it
returned a DoPutPreparedStatementResult (and the client can handle / translate
a missing message into a DoPutPreparedStatementResult)?
```suggestion
) -> Result<DoPutPreparedStatementResult, Status> {
```
Or maybe it is important to distinguish between the case where the server
didn't send back any message from the case where the server sent back a message
with an empty prepared handle?
##########
arrow-flight/src/sql/client.rs:
##########
@@ -518,17 +520,36 @@ impl PreparedStatement<Channel> {
.await
.map_err(flight_error_to_arrow_error)?;
- self.flight_sql_client
+ let result = self
+ .flight_sql_client
.do_put(stream::iter(flight_data))
.await?
- .try_collect::<Vec<_>>()
+ .message()
.await
- .map_err(status_to_arrow_error)?;
+ .map_err(status_to_arrow_error)?
+ .unwrap();
+ // Attempt to update the stored handle with any updated handle in
the DoPut result.
+ // Not all servers support this, so ignore any errors when
attempting to decode.
Review Comment:
I think this might silently ignore legitimate errors and I think it would be
a good way to distinguish between a server not sending a response vs sending an
invalid response.
If the server doesn't support sending back an updated handle, the
put_results should be empty, right? So perhaps we could check for an empty
response rather than ignoring the error?
##########
arrow-flight/src/sql/server.rs:
##########
@@ -397,11 +398,15 @@ pub trait FlightSqlService: Sync + Send + Sized + 'static
{
}
/// Bind parameters to given prepared statement.
+ ///
+ /// Returns an opaque handle that the client should pass
Review Comment:
```suggestion
/// Note that `DoPutPreparedStatementResult` contains an optional
/// opaque handle that the client should pass
```
##########
arrow-flight/tests/flight_sql_client_cli.rs:
##########
@@ -274,10 +303,17 @@ impl FlightSqlService for FlightSqlServiceImpl {
cmd: CommandPreparedStatementQuery,
_request: Request<FlightDescriptor>,
) -> Result<Response<FlightInfo>, Status> {
- assert_eq!(
- cmd.prepared_statement_handle,
- PREPARED_STATEMENT_HANDLE.as_bytes()
- );
+ if self.stateless_prepared_statements {
Review Comment:
nice
--
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]
