alamb commented on code in PR #8916:
URL: https://github.com/apache/arrow-rs/pull/8916#discussion_r2607612847
##########
arrow-flight/src/sql/client.rs:
##########
@@ -680,99 +584,10 @@ impl PreparedStatement<Channel> {
}
}
-fn decode_error_to_arrow_error(err: prost::DecodeError) -> ArrowError {
- ArrowError::IpcError(err.to_string())
-}
-
-fn status_to_arrow_error(status: tonic::Status) -> ArrowError {
- ArrowError::IpcError(format!("{status:?}"))
-}
-
-fn flight_error_to_arrow_error(err: FlightError) -> ArrowError {
- match err {
- FlightError::Arrow(e) => e,
- e => ArrowError::ExternalError(Box::new(e)),
- }
-}
-
/// A polymorphic structure to natively represent different types of data
contained in `FlightData`
pub enum ArrowFlightData {
/// A record batch
RecordBatch(RecordBatch),
/// A schema
Schema(Schema),
}
-
-/// Extract `Schema` or `RecordBatch`es from the `FlightData` wire
representation
-pub fn arrow_data_from_flight_data(
Review 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:
-
https://github.com/apache/arrow-rs?tab=readme-ov-file#deprecation-guidelines
##########
arrow-flight/src/sql/client.rs:
##########
@@ -156,7 +148,7 @@ impl FlightSqlServiceClient<Channel> {
/// If the server returns an "authorization" header, it is automatically
parsed and set as
/// 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> {
Review Comment:
this is a breaking API change I believe -- the result used to be ArrowError
and is now `FlightError`
##########
arrow-flight/src/sql/client.rs:
##########
@@ -168,18 +160,14 @@ impl FlightSqlServiceClient<Channel> {
.map_err(|_| ArrowError::ParseError("Cannot parse
header".to_string()))?;
req.metadata_mut().insert("authorization", val);
let req = self.set_request_headers(req)?;
- let resp = self
- .flight_client
- .handshake(req)
- .await
- .map_err(|e| ArrowError::IpcError(format!("Can't handshake
{e}")))?;
+ let resp = self.flight_client.handshake(req).await?;
Review 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. Could you please add it back?
--
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]
