-
Notifications
You must be signed in to change notification settings - Fork 924
Implement do_put_statement_ingest handler in FlightSqlService #6137
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
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 @djanderson 🙏
I wonder if you could also add a test for this code to show all the plumbing is hooked up correctly
Perhaps you could follow the model in https://github.com/apache/arrow-rs/blob/master/arrow-flight/tests/flight_sql_client.rs (thanks to @lewiszlw for setting up that pattern)
ba60aa3
to
acf331a
Compare
Dispatch this handler for the new CommandStatementIngest command.
acf331a
to
b505037
Compare
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
BTW when you next get a chance to work on this PR, can you please change it so it targets the |
@alamb of course. I'm working on it, I'll open a PR against |
It appears that github automatically closes the PR when the target branch is deleted -- sorry about that |
Which issue does this PR close?
Closes #6124.
Depends on #6133.
Rationale for this change
This implements the handler for the new
CommandStatementIngest
message defined in Arrow Flight SQL version 17.0.What changes are included in this PR?
The generated
CommandStatementIngest
from #6133 is exposed as a public member of the arrow_flightsql
module and theFlightSqlService
trait is updated with a handler for that new command.Are there any user-facing changes?
Yes, the trait
FlightSqlService
has a new method,do_put_statement_ingest
.This PR should be tagged
api-change
.