-
Notifications
You must be signed in to change notification settings - Fork 967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve flight sql examples #10432
Improve flight sql examples #10432
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @lewiszlw -- this looks really nice to me
@@ -246,27 +276,6 @@ impl FlightSqlService for FlightSqlServiceImpl { | |||
Ok(Response::new(Box::pin(stream))) | |||
} | |||
|
|||
async fn get_flight_info_statement( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this code means it falls back to the default implementation (which also returns Not implemented)
Seems like a nice change to me 👍
info!("get_flight_info_schemas"); | ||
Err(Status::unimplemented("Implement get_flight_info_schemas")) | ||
} | ||
|
||
async fn get_flight_info_tables( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
let fetch = FetchResults { handle: uuid }; | ||
let buf = fetch.as_any().encode_to_vec().into(); | ||
let ticket = Ticket { ticket: buf }; | ||
let endpoint = FlightEndpoint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think you can make this code simpler using FlightInfo::new()
as shown on the examples in https://docs.rs/arrow-flight/latest/arrow_flight/struct.FlightInfo.html
// Create a new FlightInfo
let flight_info = FlightInfo::new()
// Encode the Arrow schema
.try_with_schema(&schema))
.expect("encoding failed")
.with_endpoint(
FlightEndpoint::new()
.with_ticket(ticket)
)
)
.with_descriptor(FlightDescriptor::new_cmd(Default::default()));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
.with_descriptor(FlightDescriptor::new_cmd(Default::default()))
can not be compiled due to
cannot infer type for type parameter `impl Into<Bytes>` declared on the associated function `new_cmd`
So I changed to
let info = FlightInfo::new()
// Encode the Arrow schema
.try_with_schema(&schema)
.expect("encoding failed")
.with_endpoint(FlightEndpoint::new().with_ticket(ticket))
.with_descriptor(FlightDescriptor {
r#type: DescriptorType::Cmd.into(),
cmd: Default::default(),
path: vec![],
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks agian @lewiszlw
Err(Status::unimplemented( | ||
"Implement do_put_prepared_statement_query", | ||
)) | ||
let info = FlightInfo::new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
Which issue does this PR close?
Implement
get_flight_info_tables
and remove unimplemented methods.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?