Update FlightSqlService trait to proxy handshake
#2211
Conversation
arrow-flight/src/sql/server.rs
Outdated
| async fn do_handshake( | ||
| &self, | ||
| request: Request<Streaming<HandshakeRequest>>, | ||
| ) -> Result< | ||
| Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>, | ||
| Status, | ||
| >; |
There was a problem hiding this comment.
Should we provide default implementation that returns "Not yet implemented" error?
There was a problem hiding this comment.
I'm still learning Rust. Would this be done via impl specialization?
There was a problem hiding this comment.
Oh wow, I just threw the default implementation right on the trait and it compiled! I had no idea that would work nowadays 😄
There was a problem hiding this comment.
Yea, as you see, you can "override" default implementation in a trait nowadays.
Codecov Report
@@ Coverage Diff @@
## master #2211 +/- ##
==========================================
- Coverage 82.56% 82.53% -0.04%
==========================================
Files 239 239
Lines 62269 62299 +30
==========================================
+ Hits 51415 51418 +3
- Misses 10854 10881 +27
Help us with your feedback. Take ten seconds to tell us how you rate us. |
| @@ -256,9 +270,10 @@ where | |||
|
|
|||
| async fn handshake( | |||
There was a problem hiding this comment.
Yeah, this isn't cool that the impl doesn't call the underlying implementation
| let output = futures::stream::iter(vec![result]); | ||
| return Ok(Response::new(Box::pin(output))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks good 👍
It would be awesome to get some test coverage of this code eventually 🤔
There was a problem hiding this comment.
I like to follow acceptance test driven development when possible. Do you think the rust FlightSqlClient is far enough along to integration test with?
There was a problem hiding this comment.
Agreed that we eventually should add some test coverage for flight sql server. Currently we don't have any test coverage for it, not just this PR.
There was a problem hiding this comment.
When we have TPC-H up and running, I'll come back around to this and look at the main issues: cloning the server state & testing. Thanks for your feedback and helping get it merged!
There was a problem hiding this comment.
I like to follow acceptance test driven development when possible. Do you think the rust FlightSqlClient is far enough along to integration test with?
I think there is also a place for targeted for regression tests (so that, for example, if someone breaks the code accidentally in some future refactoring, they also get a test failure). However, the right balance and where to draw the line between the two is always a matter of judgement
There was a problem hiding this comment.
Sorry, I didn't mean to imply integration tests were the only way. I think they become exponentially impossible to maintain as code coverage increases - so unit tests are also required. I was just (and still am) hoping to start with a FlighSql-rs test then add unit tests in addition to that.
There was a problem hiding this comment.
I believe we are in total agreement
|
Benchmark runs are scheduled for baseline = 48cc6c3 and contender = acd8042. acd8042 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 #2210.
Rationale for this change
Apache Arrow Ballista now supports FlightSQL, and can run simple statements like
select 1;, however due to its architecture, tables need to be registered before any substantive queries can be run. Unfortunately, tables have to be registered on a context, or else the next query will have no idea that they were registered. The native Ballista protocol uses a custom field to implement this behavior, but for FlightSQL, we should use its native method. However the existing FlightSqlService trait forces this to return an unimplemented Err.What changes are included in this PR?
Proxying of the
handshake()methodAre there any user-facing changes?
Other implementers ofFlightSqlServicewill have to implement the method (they can just returnNotImplementedas that was the previous behavior).