Change FlightSQLClient to return FlightError & cleanup code
#8916
Conversation
github-actions
bot
added
arrow
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @lewiszlw -- this does make things look nicer 👌
Since it is an API change (change of error types) we'll have to wait for the next major release.
Could you move the change to arrow_data_from_flight_data into its own PR (and deprecate it first, rather than just remove it, along with a note about what API to use instead?
arrow-flight/src/sql/client.rs
Outdated
| .handshake(req) | ||
| .await | ||
| .map_err(|e| ArrowError::IpcError(format!("Can't handshake {e}")))?; | ||
| let resp = self.flight_client.handshake(req).await?; |
There was a problem hiding this comment.
I think adding context that the error was happening in the the handshake (rather than just passing along the generic service laer) was useful context, but has been lost in this refactoring.
Is there any way to add it back without losing access to the inner tonic status?
There was a problem hiding this comment.
I can't find easy way to do this. Restored this change first.
| /// a token for future requests. Any other data returned by the server in the handshake | ||
| /// response is returned as a binary blob. | ||
| pub async fn handshake(&mut self, username: &str, password: &str) -> Result<Bytes, ArrowError> { | ||
| pub async fn handshake(&mut self, username: &str, password: &str) -> Result<Bytes> { |
There was a problem hiding this comment.
this is a breaking API change I believe -- the result used to be ArrowError and is now FlightError
There was a problem hiding this comment.
Yes. FlightSqlServiceClient's methods now return FlightError instead of ArrowError.
| } | ||
|
|
||
| /// Extract `Schema` or `RecordBatch`es from the `FlightData` wire representation | ||
| pub fn arrow_data_from_flight_data( |
There was a problem hiding this comment.
This is a public function - https://docs.rs/arrow-flight/latest/arrow_flight/sql/client/fn.arrow_data_from_flight_data.html
I recommend we deprecate it first and then remove it in the future:
alamb
changed the title
FlightError & cleanup codeFlightError & cleanup code
2 participants
Which issue does this PR close?
None.
Rationale for this change
FlightClient.tonic::Status) to know if we should retry, but ArrowError stores string oftonic::Status.What changes are included in this PR?
FlightError.Are these changes tested?
CI
Are there any user-facing changes?
Yes.