FlightSQL Client & integration test #3207

avantgardnerio · 2022-11-27T17:41:56Z

Which issue does this PR close?

Closes #3206.
Closes #1413

Rationale for this change

Described in issue.

What changes are included in this PR?

A Flight SQL client & client server integration test.

Are there any user-facing changes?

The client & test.

Additional notes

This is largely based off of work done by @wangfenjin

avantgardnerio · 2022-11-27T17:42:12Z

CC @stuartcarnie @alamb

martin-g

Some minor comments from me below.
IMO all .map_err(|_| ...) should be improved to keep the original error, so that it is easier for the developer/user to debug issues.

Good work, @avantgardnerio !

arrow-flight/examples/flight_sql_server.rs

arrow-flight/src/utils.rs

martin-g · 2022-11-27T22:06:13Z

arrow-flight/src/sql/client.rs

+
+impl FlightSqlServiceClient {
+    pub async fn new_with_ep(host: &str, port: u16) -> Result<Self, ArrowError> {
+        let addr = format!("http://{}:{}", host, port);


Does it work with HTTPS ?

filed #3309 to track

martin-g · 2022-11-27T22:07:21Z

arrow-flight/src/sql/client.rs

+        let addr = format!("http://{}:{}", host, port);
+        let endpoint = Endpoint::new(addr)
+            .map_err(|_| ArrowError::IoError("Cannot create endpoint".to_string()))?
+            .connect_timeout(Duration::from_secs(20))


All these settings should probably be passed as parameters, e.g. ClientOptions.

I don't know why they are there, TBH. This code appears to have been copy & pasted from Ballista. I'd suggest we remove them and accept the defaults here and let users call new(channel) if they need extra configuration customization.

I agree it would be much nicer to avoid hard coded defaults.

One classic pattern would be a client builder

let client = FlightClient::builder() .with_timeout(Duration....) .build() .await?

However, I think we could do this as a follow on PR as well

arrow-flight/src/sql/client.rs

alamb · 2022-11-28T18:47:47Z

Thank you @avantgardnerio -- FYI I believe that @tustvold has this one on his list to review carefully in the upcoming days

alamb

Thank you @avantgardnerio -- sorry for the delay in review.

I think the flightSQL parts of this PR could be merged with some small documentation touchups

I am worried about unintended fallout from the miscellaneous other changes in this PR.

Thus, I suggest:

Mark the API as "experimental" (perhaps just put a note in the docstrings)
Break this PR into the "flight" part and the "misc cleanup" part
File a follow on ticket for client configuration API

I am happy to do any/all of the above to get this PR moving. Given our release schedule calls for an RC to be cut later this week, I hope to help get this PR in by then.

arrow-csv/src/reader.rs

alamb · 2022-12-05T18:21:27Z

arrow-flight/examples/flight_sql_server.rs

+        let uds = UnixListener::bind(path.clone()).unwrap();
+        let stream = UnixListenerStream::new(uds);
+
+        // We would just listen on TCP, but it seems impossible to know when tonic is ready to serve


We got around this limitation in IOx by polling (as in try a request in a loop, if it fails, sleep for a while and try again, with an eventual timeout)

I wish they would add a method where you can await actual start, or a oneshot channel, or something, but I guess this is where we are.

I also like this approach because it allows the tests to run in parallel. I'm open to either way, I don't have a strong opinion here.

me neither -- this is fine with me

arrow-flight/examples/flight_sql_server.rs

arrow-select/src/take.rs

alamb · 2022-12-05T18:27:28Z

arrow-flight/src/sql/client.rs

+        let addr = format!("http://{}:{}", host, port);
+        let endpoint = Endpoint::new(addr)
+            .map_err(|_| ArrowError::IoError("Cannot create endpoint".to_string()))?
+            .connect_timeout(Duration::from_secs(20))


I agree it would be much nicer to avoid hard coded defaults.

One classic pattern would be a client builder

let client = FlightClient::builder() .with_timeout(Duration....) .build() .await?

However, I think we could do this as a follow on PR as well

arrow-flight/src/sql/client.rs

avantgardnerio · 2022-12-07T21:13:00Z

@alamb I think I addressed the 3 main points above. Hopefully this makes it "merge ready" and the remaining issues can be handled in follow-on PRs.

alamb

Thank you so much @avantgardnerio

I think this PR represents a major step forward for FlightSQL and could me merged as is, even though there are several significant items left. The only thing I think we should remove prior to releasing the next arrow-rs is the new tokio-stream dependency (which I am happy to make a PR)

Since all this code is behind the --flight-sql-experimental flag I don't think this will impact downstream consumers.

Thus, I would like to merge it in so we can work on the other items in parallel and will start an organziational ticket to track the work

@tustvold / @viirya do you object to merging this PR?

arrow-flight/Cargo.toml

alamb · 2022-12-08T18:40:06Z

arrow-flight/examples/flight_sql_server.rs

+    }
+
+    #[tokio::test]
+    async fn test_select_1() {


This test does not appear to actually run as part of cargo test --all

The only way I could get it to run is like:

cargo test -p arrow-flight --features=flight-sql-experimental --examples

Which does not appear to run as part of CI

alamb · 2022-12-08T20:02:05Z

I plan to merge this PR tomorrow (prior to cutting the next arrow-rs release) unless I hear otherwise

I am starting to gather / plan FlightSQL support in #3301

arrow-flight/src/utils.rs

arrow-flight/src/sql/client.rs

viirya · 2022-12-08T21:11:27Z

arrow-flight/src/sql/client.rs

+            .timeout(Duration::from_secs(20))
+            .tcp_nodelay(true) // Disable Nagle's Algorithm since we don't want packets to wait
+            .tcp_keepalive(Option::Some(Duration::from_secs(3600)))
+            .http2_keep_alive_interval(Duration::from_secs(300))
+            .keep_alive_timeout(Duration::from_secs(20))
+            .keep_alive_while_idle(true);


I think we may come with a option struct and have these as default values. And users can specify them if needed.

I personally prefer that approach.

Filed #3308 for this item

arrow-flight/src/sql/client.rs

alamb · 2022-12-08T20:04:34Z

arrow-flight/Cargo.toml

@@ -45,6 +45,10 @@ default = []
 flight-sql-experimental = ["prost-types"]

 [dev-dependencies]
+arrow = { version = "28.0.0", path = "../arrow", features = ["prettyprint"] }
+tempfile = "3.3"
+tokio-stream = { version = "0.1", features = ["net"] }


alamb · 2022-12-09T14:40:37Z

I believe I have filed tickets for all follow ons identified in this PR. They are collected under #3301

alamb · 2022-12-09T14:40:58Z

Thanks @avantgardnerio for the contribution and @martin-g and @viirya for the review!

avantgardnerio · 2022-12-09T15:23:25Z

Thanks @avantgardnerio

My pleasure... I'm glad we can finally start writing automated tests for FlightSQL in Ballista 👍

alamb · 2022-12-09T15:56:35Z

My pleasure... I'm glad we can finally start writing automated tests for FlightSQL in Ballista 👍

🎉

As a heads up @avantgardnerio -- what I plan to do is to hack a FlightSQL implementation into IOx, likely with a temporary fork of arrow-flight and then contribute anything needed back upstream (like client configuration). I'll keep you updated

Dandandan · 2022-12-09T17:03:38Z

arrow-flight/src/utils.rs

+    let schema = Arc::new(schema);
+
+    let mut batches = vec![];
+    let dictionaries_by_id = HashMap::new();


This doesn't support dictionaries yet? Maybe we can turn this into an issue?

Dandandan · 2022-12-09T17:05:31Z

arrow-flight/src/utils.rs

+    let mut flight_data = vec![];
+    for batch in batches.iter() {
+        let (flight_dictionaries, flight_datum) =
+            flight_data_from_arrow_batch(batch, &options);


See #3312 - for dictionary data this is inefficient (as repeated use of the dictionary will have new FlightData instances to send over the wire.

Dandandan · 2022-12-09T17:11:06Z

arrow-flight/src/sql/client.rs

+                    )
+                })?;
+
+            let dictionaries_by_field = HashMap::new();


Same here - it should support dictionaries on the wire?

Dandandan · 2022-12-09T17:15:06Z

@avantgardnerio really cool stuff!

I added some notes on the dictionary support as this might be a big bottleneck for dictionary-encoded data.

* squash * Undo nightly clippy advice * PR feedback * PR feedback * PR feedback * PR feedback * Formatting

github-actions bot added the arrow-flight Changes to the arrow-flight crate label Nov 27, 2022

avantgardnerio changed the title ~~(Failing) client/server integration test~~ FlightSQL Client & integration test Nov 27, 2022

avantgardnerio marked this pull request as ready for review November 27, 2022 18:21

github-actions bot added arrow Changes to the arrow crate object-store Object Store Interface parquet Changes to the parquet crate labels Nov 27, 2022

martin-g reviewed Nov 27, 2022

View reviewed changes

avantgardnerio requested a review from martin-g November 28, 2022 16:12

alamb reviewed Dec 5, 2022

View reviewed changes

squash

ece7553

avantgardnerio force-pushed the bg_flightsql_client branch from db9fc38 to ece7553 Compare December 7, 2022 20:53

Undo nightly clippy advice

d373db8

github-actions bot removed parquet Changes to the parquet crate arrow Changes to the arrow crate object-store Object Store Interface labels Dec 7, 2022

Brent Gardner added 2 commits December 7, 2022 14:10

PR feedback

defa990

PR feedback

223e6c8

avantgardnerio requested review from alamb and removed request for martin-g December 7, 2022 21:50

alamb approved these changes Dec 8, 2022

View reviewed changes

PR feedback

66c29a5

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/utils.rs Outdated Show resolved Hide resolved

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/sql/client.rs Outdated Show resolved Hide resolved

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/sql/client.rs Outdated Show resolved Hide resolved

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/sql/client.rs Outdated Show resolved Hide resolved

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/sql/client.rs Show resolved Hide resolved

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/sql/client.rs Outdated Show resolved Hide resolved

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/sql/client.rs Outdated Show resolved Hide resolved

viirya reviewed Dec 8, 2022

View reviewed changes

arrow-flight/src/sql/client.rs Outdated Show resolved Hide resolved

Brent Gardner added 2 commits December 8, 2022 16:40

PR feedback

5f6db7c

Formatting

7406513

alamb approved these changes Dec 9, 2022

View reviewed changes

This was referenced Dec 9, 2022

[FlightSQL] Add Builder for configuring options on flight sql client #3308

Open

[FlightSQL] Support HTTPs #3309

Closed

[FlightSQL] Client Allow users to set arbitrary headers (rather than only the "authorization") #3310

Closed

alamb merged commit d18827d into apache:master Dec 9, 2022

avantgardnerio deleted the bg_flightsql_client branch December 9, 2022 15:22

Dandandan reviewed Dec 9, 2022

View reviewed changes

alamb pushed a commit to alamb/arrow-rs that referenced this pull request Dec 9, 2022

FlightSQL Client & integration test (apache#3207)

7530578

* squash * Undo nightly clippy advice * PR feedback * PR feedback * PR feedback * PR feedback * Formatting

alamb mentioned this pull request Dec 9, 2022

WIP FlightSQL Integration Branch #3316

Closed

tustvold mentioned this pull request Dec 19, 2022

Implement Basic FlightSQL Client #1413 #1616

Closed

alamb mentioned this pull request Dec 19, 2022

Mid-level ArrowFlight Client #3371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FlightSQL Client & integration test #3207

FlightSQL Client & integration test #3207

avantgardnerio commented Nov 27, 2022 •

edited by alamb

avantgardnerio commented Nov 27, 2022

martin-g left a comment

martin-g Nov 27, 2022

avantgardnerio Nov 28, 2022

alamb Dec 9, 2022

martin-g Nov 27, 2022

avantgardnerio Nov 28, 2022

alamb Dec 5, 2022

alamb commented Nov 28, 2022

alamb left a comment •

edited

alamb Dec 5, 2022

avantgardnerio Dec 7, 2022

alamb Dec 8, 2022

alamb Dec 5, 2022

avantgardnerio commented Dec 7, 2022

alamb left a comment

alamb Dec 8, 2022

alamb commented Dec 8, 2022

viirya Dec 8, 2022

avantgardnerio Dec 8, 2022

alamb Dec 9, 2022

alamb Dec 8, 2022

alamb commented Dec 9, 2022

alamb commented Dec 9, 2022

avantgardnerio commented Dec 9, 2022

alamb commented Dec 9, 2022

Dandandan Dec 9, 2022

Dandandan Dec 9, 2022

Dandandan Dec 9, 2022

Dandandan commented Dec 9, 2022

FlightSQL Client & integration test #3207

FlightSQL Client & integration test #3207

Conversation

avantgardnerio commented Nov 27, 2022 • edited by alamb

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Additional notes

avantgardnerio commented Nov 27, 2022

martin-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 28, 2022

alamb left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avantgardnerio commented Dec 7, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 9, 2022

alamb commented Dec 9, 2022

avantgardnerio commented Dec 9, 2022

alamb commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dandandan commented Dec 9, 2022

avantgardnerio commented Nov 27, 2022 •

edited by alamb

alamb left a comment •

edited