You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "amitvijapur (via GitHub)" <gi...@apache.org> on 2026/09/22 21:29:01 UTC
[PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
amitvijapur opened a new pull request, #11180:
URL: https://github.com/apache/arrow-rs/pull/11180
# Which issue does this PR close?
Closes #6487.
# Rationale for this change
The Flight SQL CLI iterates the endpoints of a `FlightInfo` and calls `DoGet` with each ticket on the client it already has, ignoring `FlightEndpoint::location`. The protocol says a ticket with locations is redeemed at one of them, so a server whose `GetFlightInfo` and `DoGet` live on different addresses gets no data from the CLI.
# What changes are included in this PR?
Only the CLI (`flight_sql_client.rs`) changes. `setup_client` now delegates to `setup_client_for_uri`, which builds a client for any URI with the same headers, token, handshake and compression settings; TLS follows the URI scheme rather than `--tls`, since a location may differ from the main connection. `execute_flight` uses the existing client when an endpoint lists no location, or only the reserved `arrow-flight-reuse-connection://?` or empty-string form that the Flight spec defines as "reuse this connection", and otherwise connects to the first listed location, caching one client per distinct location for the command. A location that cannot be connected to is an error naming the URI rather than a silent fallback, because the ticket may not be redeemable on the original server.
The library `FlightClient` / `FlightSqlServiceClient` APIs are unchanged; the issue's point about those being easy to misuse is left open.
# Are these changes tested?
Two integration tests in `flight_sql_client_cli.rs`: one runs two test servers, where the first answers `GetFlightInfo` with endpoints located at the second and refuses `DoGet` itself, and asserts the CLI prints the same table as `test_simple`; the other advertises the reserved URI, then the empty string, and serves `DoGet` itself. The first fails on `main` with `DoGet is served by another server`. The second passes on `main`, which ignores locations entirely; it guards the new path and fails with `invalid URI` if either reserved form is dialled. The shared CLI invocation moved into a helper. `cargo test -p arrow-flight --features cli,flight-sql,tls-ring` passes.
# Are there any user-facing changes?
The CLI now follows endpoint locations. Servers that return endpoints without locations behave as before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #11180:
URL: https://github.com/apache/arrow-rs/pull/11180#issuecomment-6006972355
are these comments directly from an LLM?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11180:
URL: https://github.com/apache/arrow-rs/pull/11180#discussion_r4130964152
##########
arrow-flight/src/bin/flight_sql_client.rs:
##########
@@ -398,12 +430,24 @@ fn setup_logging(args: LoggingArgs) -> Result<()> {
Ok(())
}
-async fn setup_client(args: ClientArgs) -> Result<FlightSqlServiceClient<Channel>> {
+async fn setup_client(args: &ClientArgs) -> Result<FlightSqlServiceClient<Channel>> {
let port = args.port.unwrap_or(if args.tls { 443 } else { 80 });
let protocol = if args.tls { "https" } else { "http" };
- let mut endpoint = Endpoint::new(format!("{}://{}:{}", protocol, args.host, port))
+ setup_client_for_uri(args, &format!("{}://{}:{}", protocol, args.host, port)).await
+}
+
+/// Connect a client to `uri`, applying the headers, token, handshake and compression settings from
+/// `args`. TLS is used when `uri` has an `https` scheme, so that an endpoint location may differ
+/// from the main connection.
+async fn setup_client_for_uri(
+ args: &ClientArgs,
+ uri: &str,
+) -> Result<FlightSqlServiceClient<Channel>> {
+ let tls = uri.starts_with("https://");
Review Comment:
if a user specifies `tls` in the cli args, but a server responds with a location that is just `http`, it might seen surprising to just accept and use this. perhaps we should try to look only for `https` in cases where user specifies `tls`, and fail explicitly if one cannot be found 🤔
##########
arrow-flight/src/bin/flight_sql_client.rs:
##########
@@ -329,18 +337,42 @@ async fn main() -> Result<()> {
async fn execute_flight(
client: &mut FlightSqlServiceClient<Channel>,
+ client_args: &ClientArgs,
info: FlightInfo,
) -> Result<Vec<RecordBatch>> {
let schema = Arc::new(Schema::try_from(info.clone()).context("valid schema")?);
let mut batches = Vec::with_capacity(info.endpoint.len() + 1);
batches.push(RecordBatch::new_empty(schema));
info!("decoded schema");
+ let mut location_clients = HashMap::new();
+
for endpoint in info.endpoint {
let Some(ticket) = &endpoint.ticket else {
bail!("did not get ticket");
};
+ // `None` means no location was given, or only the reserved reuse-connection form, so
+ // the ticket is redeemed on the server that returned the `FlightInfo`.
+ let location = endpoint
+ .location
+ .iter()
+ .map(|location| location.uri.as_str())
+ .find(|uri| !uri.is_empty() && *uri != REUSE_CONNECTION_URI);
+
+ let client = match location {
+ None => &mut *client,
+ Some(uri) => match location_clients.entry(uri.to_owned()) {
+ Entry::Occupied(entry) => entry.into_mut(),
+ Entry::Vacant(entry) => {
+ let client = setup_client_for_uri(client_args, uri)
Review Comment:
does the spec technically require us to do authentication handshake for these endpoint servers? i tried taking a look but it seemed a bit vague
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
Posted by "amitvijapur (via GitHub)" <gi...@apache.org>.
amitvijapur commented on PR #11180:
URL: https://github.com/apache/arrow-rs/pull/11180#issuecomment-5980260568
@Jefffrey, you are right about HTTP/HTTPS extended locations too. My earlier patch and fixtures incorrectly treated those advertisements as gRPC servers. In 41643b6 the fixtures use canonical gRPC locations, and advertised HTTP download URLs are no longer passed to DoGet or given Flight credentials. Unsupported locations are skipped when a supported gRPC or explicitly reusable connection exists; otherwise the CLI reports an unsupported-location error, including HTTP endpoints without tickets. Direct GET downloads, IPC/content-type handling and separate authentication remain outside this CLI patch and need a follow-up. I corrected the description and changed Closes to Part of #6487 to make the scope explicit. This correction was prepared with Codex and separately reviewed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
Posted by "amitvijapur (via GitHub)" <gi...@apache.org>.
amitvijapur commented on code in PR #11180:
URL: https://github.com/apache/arrow-rs/pull/11180#discussion_r4137860241
##########
arrow-flight/src/bin/flight_sql_client.rs:
##########
@@ -398,12 +430,24 @@ fn setup_logging(args: LoggingArgs) -> Result<()> {
Ok(())
}
-async fn setup_client(args: ClientArgs) -> Result<FlightSqlServiceClient<Channel>> {
+async fn setup_client(args: &ClientArgs) -> Result<FlightSqlServiceClient<Channel>> {
let port = args.port.unwrap_or(if args.tls { 443 } else { 80 });
let protocol = if args.tls { "https" } else { "http" };
- let mut endpoint = Endpoint::new(format!("{}://{}:{}", protocol, args.host, port))
+ setup_client_for_uri(args, &format!("{}://{}:{}", protocol, args.host, port)).await
+}
+
+/// Connect a client to `uri`, applying the headers, token, handshake and compression settings from
+/// `args`. TLS is used when `uri` has an `https` scheme, so that an endpoint location may differ
+/// from the main connection.
+async fn setup_client_for_uri(
+ args: &ClientArgs,
+ uri: &str,
+) -> Result<FlightSqlServiceClient<Channel>> {
+ let tls = uri.starts_with("https://");
Review Comment:
Updated in 18be6d2. With `--tls`, the CLI now selects the first HTTPS location, including one listed after plaintext locations. If there is no HTTPS location, it can reuse the original TLS connection when the endpoint advertises the reuse form; otherwise it returns an explicit error. Added regression tests for these cases and preserved selection without `--tls`. The Arrow Flight crate tests and Clippy pass.
##########
arrow-flight/src/bin/flight_sql_client.rs:
##########
@@ -329,18 +337,42 @@ async fn main() -> Result<()> {
async fn execute_flight(
client: &mut FlightSqlServiceClient<Channel>,
+ client_args: &ClientArgs,
info: FlightInfo,
) -> Result<Vec<RecordBatch>> {
let schema = Arc::new(Schema::try_from(info.clone()).context("valid schema")?);
let mut batches = Vec::with_capacity(info.endpoint.len() + 1);
batches.push(RecordBatch::new_empty(schema));
info!("decoded schema");
+ let mut location_clients = HashMap::new();
+
for endpoint in info.endpoint {
let Some(ticket) = &endpoint.ticket else {
bail!("did not get ticket");
};
+ // `None` means no location was given, or only the reserved reuse-connection form, so
+ // the ticket is redeemed on the server that returned the `FlightInfo`.
+ let location = endpoint
+ .location
+ .iter()
+ .map(|location| location.uri.as_str())
+ .find(|uri| !uri.is_empty() && *uri != REUSE_CONNECTION_URI);
+
+ let client = match location {
+ None => &mut *client,
+ Some(uri) => match location_clients.entry(uri.to_owned()) {
+ Entry::Occupied(entry) => entry.into_mut(),
+ Entry::Vacant(entry) => {
+ let client = setup_client_for_uri(client_args, uri)
Review Comment:
I found no requirement to handshake on each endpoint server. The [Flight authentication section](https://arrow.apache.org/docs/format/Flight.html#authentication) explicitly allows token-based calls without a handshake, and `FlightSqlServiceClient::token()` is documented for sharing a bearer token with other `DoGet` clients. In 18be6d2, new endpoint clients copy the original client's token instead of replaying username/password. This covers both an explicit `--token` and a token issued by the initial handshake. The integration test uses a data server that requires that bearer token and rejects handshakes, and runs both authentication modes. Token acceptance across the advertised servers is still application-dependent; this does not add cookie-based session handling. This follow-up was implemented with Codex assistance and separately reviewed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11180:
URL: https://github.com/apache/arrow-rs/pull/11180#discussion_r4166953284
##########
arrow-flight/src/bin/flight_sql_client.rs:
##########
@@ -398,12 +435,73 @@ fn setup_logging(args: LoggingArgs) -> Result<()> {
Ok(())
}
-async fn setup_client(args: ClientArgs) -> Result<FlightSqlServiceClient<Channel>> {
+/// Prefer a separate location, falling back to the original connection when permitted.
+fn select_endpoint_location<'a>(
+ locations: impl IntoIterator<Item = &'a str>,
+ tls: bool,
+) -> Result<Option<&'a str>> {
+ let mut reuse_connection = false;
+ let mut has_locations = false;
+ for uri in locations {
+ has_locations = true;
+ if uri.is_empty() || uri == REUSE_CONNECTION_URI {
+ reuse_connection = true;
+ } else if !tls || uri.starts_with("https://") {
Review Comment:
should we be looking at `grpc+tls` too? for reference:
- https://arrow.apache.org/docs/format/Flight.html#location-uris
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
Posted by "amitvijapur (via GitHub)" <gi...@apache.org>.
amitvijapur commented on code in PR #11180:
URL: https://github.com/apache/arrow-rs/pull/11180#discussion_r4177747195
##########
arrow-flight/src/bin/flight_sql_client.rs:
##########
@@ -398,12 +435,73 @@ fn setup_logging(args: LoggingArgs) -> Result<()> {
Ok(())
}
-async fn setup_client(args: ClientArgs) -> Result<FlightSqlServiceClient<Channel>> {
+/// Prefer a separate location, falling back to the original connection when permitted.
+fn select_endpoint_location<'a>(
+ locations: impl IntoIterator<Item = &'a str>,
+ tls: bool,
+) -> Result<Option<&'a str>> {
+ let mut reuse_connection = false;
+ let mut has_locations = false;
+ for uri in locations {
+ has_locations = true;
+ if uri.is_empty() || uri == REUSE_CONNECTION_URI {
+ reuse_connection = true;
+ } else if !tls || uri.starts_with("https://") {
Review Comment:
You are right. Corrected in 41643b6: the CLI selects canonical grpc/grpc+tcp plaintext or grpc+tls secure locations, then maps them to tonic HTTP/HTTPS transport URIs only when constructing the connection. grpc+tls enables TLS even without --tls; --tls still excludes plaintext-only choices. Connection reuse and the established bearer token are preserved. A wire-level regression detects the original plaintext HTTP/2 preface and verifies a TLS ClientHello after the correction. It tests TLS initiation, not a successful TLS session or certificate validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-flight): send DoGet to the endpoint location in the Flight SQL CLI [arrow-rs]
Posted by "amitvijapur (via GitHub)" <gi...@apache.org>.
amitvijapur commented on PR #11180:
URL: https://github.com/apache/arrow-rs/pull/11180#issuecomment-6251189436
Yes, Codex drafted those comments, and I posted them as written. AI also assisted with the implementation and tests. I'm reviewing the implementation and tests myself; that review is still in progress. I'll write further explanations in my own words and be clear about what I've personally checked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org