Pass pull Request<FlightDescriptor> to FlightSqlService impls
#2309
Conversation
github-actions
bot
added
arrow-flight
|
CC @andygrove and @alamb |
arrow-flight/src/sql/server.rs
Outdated
| ) -> Result<Response<FlightInfo>, Status>; | ||
|
|
||
| /// Get a FlightInfo for executing an already created prepared statement. | ||
| async fn get_flight_info_prepared_statement( | ||
| &self, | ||
| query: CommandPreparedStatementQuery, | ||
| request: FlightDescriptor, | ||
| auth: &Option<String> |
There was a problem hiding this comment.
Adding this to all the methods is the API change.
avantgardnerio
changed the title
FlightSqlService impls
|
cc @wangfenjin who contributed the initial FlightSQL work |
|
Also, for other reviewers, a whitespace blind diff makes this PR much smaller: https://github.com/apache/arrow-rs/pull/2309/files?w=1 and palatable for review 😅 |
|
Also cc @timvw |
Codecov Report
@@ Coverage Diff @@
## master #2309 +/- ##
==========================================
- Coverage 81.26% 80.61% -0.65%
==========================================
Files 248 248
Lines 60605 59905 -700
==========================================
- Hits 49249 48295 -954
- Misses 11356 11610 +254
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
avantgardnerio
force-pushed
the
bg_sessions
branch
from
bffe4af
to
44b0a4f
Compare
avantgardnerio
force-pushed
the
bg_sessions
branch
from
44b0a4f
to
4e28be7
Compare
|
We need to check the implementation in c or other languages, seems they use something like middleware to do this, we need to make sure rust implementation same with others. Pass meta to every handler function this way might not be a good idea. |
|
Sorry I didn't pay attention to the auth part in the initial implementation... |
arrow-flight/src/sql/server.rs
Outdated
| @@ -66,76 +67,87 @@ pub trait FlightSqlService: | |||
| &self, | |||
| query: CommandStatementQuery, | |||
| request: FlightDescriptor, | |||
| metadata: MetadataMap, | |||
There was a problem hiding this comment.
I wonder if it might be better to modify request to be Request<FlightDescriptor>. Not only would this provide access to the metadata map, but also things like extensions. I think it would also avoid a clone
In particular extensions are a common way to allow middleware to extract data from the headers, and make it easily accessible to downstreams. As an example in IOx, we use a middleware to extract tracing information from requests - https://github.com/influxdata/influxdb_iox/blob/main/trace_http/src/tower.rs
I could see the
The only other way I can think of doing it is with something like a
No worries, thanks for getting it started! I'd have been lost without the foundation you laid :) |
There was a problem hiding this comment.
I think the interface is looking quite nice now 👌 Very straightforward.
Thanks @tustvold @avantgardnerio and @wangfenjin for the discussion
|
Looks like there are a few more CI checks to clean up but I think this PR is quite close |
alamb
changed the title
FlightSqlService implsRequest<FlightDescriptor> to FlightSqlService impls
There was a problem hiding this comment.
I think this looks like a great improvement. Thank you @avantgardnerio
|
I'll plan to merge this tomorrow before making an arrow 20.0.0 release candidate unless someone objects or would like more time to review |
|
Benchmark runs are scheduled for baseline = 2683b06 and contender = 0af81e8. 0af81e8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2308.
Rationale for this change
Make it so Ballista can run non-trivial FlightSql queries.
What changes are included in this PR?
Not-ready-for mergecode that I was successfully able to use to:select top 1 c_name from customer order by c_name(in my own custom JDBC driver)Are there any user-facing changes?
FlightSql queries become useful and JDBC works.