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

Better flight SQL example codes #4144

Merged
merged 5 commits into from
Apr 28, 2023
Merged

Better flight SQL example codes #4144

merged 5 commits into from
Apr 28, 2023

Conversation

sundy-li
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

Provide more detail examples and test codes for flight SQL.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Apr 27, 2023
@sundy-li
Copy link
Contributor Author

sundy-li commented Apr 27, 2023

I profiled the test case test_select_1 and found that prepare stmt cause much more latency in TcpClient, no ideas about the reason.

 cargo test --release --package arrow-flight --example flight_sql_server --all-features --features flight-sql-experimental --features tls -- tests::test_select_1 --exact --nocapture 

running 1 test
testing uds client
prepare cost: 761.35µs
execute cost: 671.36µs
do get cost: 716.169µs
Client finished!
=======
testing tcp client
prepare cost: 41.503622ms
execute cost: 1.55827ms
do get cost: 1.58378ms
Client finished!
=======
testing https client
prepare cost: 1.39358ms
execute cost: 1.31692ms
do get cost: 1.354041ms
Client finished!
=======
test tests::test_select_1 ... ok

@sundy-li
Copy link
Contributor Author

sundy-li commented Apr 27, 2023

no ideas about the reason.

Oh, it's due to TcpNoDelay, it's not enabled. databendlabs/databend#11252

Copy link
Contributor

@tustvold tustvold 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 to me, mostly just some minor nits. I took the liberty of creating #4145 which should remove the need for the once_cell shenanigans. Happy to update this PR once that is in.

{
let addr = "127.0.0.1:50051";

// We would just listen on TCP, but it seems impossible to know when tonic is ready to serve
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
// We would just listen on TCP, but it seems impossible to know when tonic is ready to serve

I believe the sleep below is to handle this

.serve(addr.parse().unwrap());

let request_future = async {
sleep(Duration::from_millis(2000)).await;
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
sleep(Duration::from_millis(2000)).await;
// wait for server to start up
sleep(Duration::from_millis(2000)).await;

F: FnOnce(FlightSqlServiceClient<Channel>) -> C,
C: Future<Output = ()>,
{
let addr = "127.0.0.1:50051";
Copy link
Contributor

Choose a reason for hiding this comment

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

#4145 shows how this could be adapted to not need to hard-code a port

@@ -34,17 +34,37 @@ arrow-cast = { workspace = true }
arrow-ipc = { workspace = true }
arrow-schema = { workspace = true }
base64 = { version = "0.21", default-features = false, features = ["std"] }
tonic = { version = "0.9", default-features = false, features = ["transport", "codegen", "prost"] }
tonic = { version = "0.9", default-features = false, features = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally find this formatting harder to read...

@tustvold
Copy link
Contributor

Working on updating this

@tustvold tustvold merged commit b717b39 into apache:master Apr 28, 2023
@tustvold
Copy link
Contributor

Thank you for this 👍

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
Development

Successfully merging this pull request may close these issues.

2 participants