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

fix: flight examples #9335

Merged
merged 5 commits into from
Feb 26, 2024
Merged

fix: flight examples #9335

merged 5 commits into from
Feb 26, 2024

Conversation

codonnell
Copy link
Contributor

Which issue does this PR close?

Closes #9334.

Rationale for this change

It allows the arrow flight examples to compile and makes all nested examples runnable.

What changes are included in this PR?

This PR downgrades the tonic dependency to 0.10 to match arrow-flight and adds explicit [[example]] blocks so nested examples can be run with cargo run --example.

Are these changes tested?

I verified locally that the arrow flight examples compile with this change, and I was able to run queries with JDBC against the arrow_flight_sql example.

Are there any user-facing changes?

No, just example fixes.

Tonic 0.10 and 0.11 are not API compatible.
Arrow 50 depends on tonic 0.10, and datafusion must match that dependency for compatibility reasons.
cargo run --example doesn't support nested examples. Nested examples need an explicit block to be runnable.
@@ -54,6 +78,6 @@ serde = { version = "1.0.136", features = ["derive"] }
serde_json = { workspace = true }
tempfile = { workspace = true }
tokio = { workspace = true, features = ["rt-multi-thread", "parking_lot"] }
tonic = "0.11"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change related?

Copy link
Contributor Author

@codonnell codonnell Feb 24, 2024

Choose a reason for hiding this comment

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

I tried to capture this in the issue:

There is an API incompatibility between tonic 0.10 and 0.11. We have 0.10 as a transitive dependency of arrow-flight, and datafusion-examples pulls in 0.11 directly. As a result the flight examples won't compile.

If that's not clear, I'm happy to clarify further.

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, thanks for this

Do we run (or at least build) these examples as part of the CI? Since if so that should've prevented the bump in the first place: #9176

arrow-flight will have tonic version 0.11 in it's next release as it was also bumped: apache/arrow-rs#5380

So we can downgrade for now until arrow 51.0.0 is released

@codonnell
Copy link
Contributor Author

codonnell commented Feb 25, 2024

Do we run (or at least build) these examples as part of the CI? Since if so that should've prevented the bump in the first place: #9176

I wondered about that, too. The flight examples were excluded in this refactor, presumably because the server examples run indefinitely and the client example needs a server to connect to. We could add a cargo check for all the examples to the CI script; I'm not sure why we wouldn't. With this PR, cargo check passes for all the examples. I'm happy to add that if you think it would be valuable.

arrow-flight will have tonic version 0.11 in it's next release as it was also bumped: apache/arrow-rs#5380

So we can downgrade for now until arrow 51.0.0 is released

My thoughts, exactly. 👍

@codonnell
Copy link
Contributor Author

Is it worth adding a short comment above the tonic dependency explaining the situation? Might help whoever upgrades arrow understand how to resolve the tonic incompatibility more quickly.

@codonnell
Copy link
Contributor Author

I went ahead and added the CI check and comment. Happy to adjust as needed.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @codonnell

@comphead comphead merged commit c568407 into apache:main Feb 26, 2024
24 checks passed
@codonnell codonnell deleted the fix-flight-examples branch February 27, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arrow flight examples don't compile
3 participants