Skip to content
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

Arrow Flight SQL example JDBC driver incompatibility #5666

Merged

Conversation

istvan-fodor
Copy link
Contributor

@istvan-fodor istvan-fodor commented Apr 18, 2024

Which issue does this PR close?

Closes #5665
Closes #5669

Rationale for this change

It would be great if the example Flight SQL server would support integration with the Arrow JDBC driver. This would demonstrate the cross-platform nature of Arrow quite well.

This PR changes the example flight_sql_server so we can query it from Java using the Arrow JDBC driver.

What changes are included in this PR?

  1. authorization header is returned with contents "Bearer "
  2. Added clear_token() to FlightSqlServiceClient to give the client the ability to re-authenticate.
  3. The location field returned by get_flight_info_prepared_statement uses a "grpc+tcp://127.0.0.1" as its location. Instead it should include the host and the port.
  4. do_action_close_prepared_statement needs to include at least an empty stub (return Ok(())).

Are there any user-facing changes?

No

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Apr 18, 2024
@istvan-fodor istvan-fodor changed the title feat: do_handshake returns auth header + clear_token function on sql client Arrow Flight SQL example JDBC driver incompatibility Apr 18, 2024
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

arrow-flight/examples/flight_sql_server.rs Outdated Show resolved Hide resolved
arrow-flight/examples/flight_sql_server.rs Outdated Show resolved Hide resolved
arrow-flight/examples/flight_sql_server.rs Show resolved Hide resolved
Cleaner type for response variable

Co-authored-by: Jeffrey Vo <[email protected]>
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might have to separate the listening address vs the server address to make things clearer 🤔

Thoughts on this?

@jduo
Copy link
Member

jduo commented Apr 23, 2024

We might have to separate the listening address vs the server address to make things clearer 🤔

Thoughts on this?

If you have FlightInfo return an empty location, clients can re-use the connection that was used for the GetFlightInfo request. This is useful for the case where returning this IP/host is non-trivial.

@istvan-fodor
Copy link
Contributor Author

istvan-fodor commented Apr 23, 2024

We might have to separate the listening address vs the server address to make things clearer 🤔
Thoughts on this?

If you have FlightInfo return an empty location, clients can re-use the connection that was used for the GetFlightInfo request. This is useful for the case where returning this IP/host is non-trivial.

I can add this change, and just return an empty list - it seems to me the best default case that makes sense for most users. The FlightEndpoint struct has good comments about this too.

Copy link
Contributor Author

@istvan-fodor istvan-fodor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added the change suggested by @jduo and removed location from the returned FlightEndpoint

@istvan-fodor istvan-fodor requested a review from Jefffrey April 23, 2024 14:06
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Comment on lines +735 to +736
let addr_str = "0.0.0.0:50051";
let addr = addr_str.parse()?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let addr_str = "0.0.0.0:50051";
let addr = addr_str.parse()?;
let addr = "0.0.0.0:50051".parse()?;
let svc = FlightServiceServer::new(FlightSqlServiceImpl {});

Minor nit, but we can revert to what it was previously (and down below too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
4 participants