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

Why exclude datafusion-cli from workspace? #6577

Closed
r4ntix opened this issue Jun 7, 2023 · 4 comments · Fixed by #6600
Closed

Why exclude datafusion-cli from workspace? #6577

r4ntix opened this issue Jun 7, 2023 · 4 comments · Fixed by #6600
Labels
bug Something isn't working

Comments

@r4ntix
Copy link
Contributor

r4ntix commented Jun 7, 2023

Describe the bug

The tests for datafusion-cli is failing in the current main branch, but our CI doesn't find it.

$ cargo test
   Compiling datafusion-cli v26.0.0 (/Users/r4ntix/Workspace/github/arrow-datafusion/datafusion-cli)
error[E0277]: the `?` operator can only be applied to values that implement `Try`
   --> datafusion-cli/src/exec.rs:251:17
    |
251 |                 create_external_table(&ctx, cmd)?;
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `?` operator cannot be applied to type `impl Future<Output = std::result::Result<(), datafusion::error::DataFusionError>>`
    |
    = help: the trait `Try` is not implemented for `impl Future<Output = std::result::Result<(), datafusion::error::DataFusionError>>`
help: consider `await`ing on the `Future`
    |
251 |                 create_external_table(&ctx, cmd).await?;
    |                                                 ++++++

For more information about this error, try `rustc --explain E0277`.
error: could not compile `datafusion-cli` due to previous error

I noticed that we currently exclude datafusion-cli from workspace.

To Reproduce

$ cd datafusion-cli
$ cargo test

Expected behavior

No response

Additional context

No response

@r4ntix r4ntix added the bug Something isn't working label Jun 7, 2023
@r4ntix
Copy link
Contributor Author

r4ntix commented Jun 7, 2023

The current test bug of datafusion-cli has been fixed in #6576 .
But should we add datafusion-cli to workspace to guarantee that the tests are covered?

@jiangzhx
Copy link
Contributor

jiangzhx commented Jun 7, 2023

HaHa
i find the PR to exclude datafusion-cli. hope it's help for you.

and there is a issues want to add datafusion-cli back to workspace.

@alamb
Copy link
Contributor

alamb commented Jun 7, 2023

The current test bug of datafusion-cli has been fixed in #6576 .

Thank you!

But should we add datafusion-cli to workspace to guarantee that the tests are covered?

There were reasons (related to publishing datafusion-cli as a package -- #2071 -- as I remember 🤔 ) for not putting it i the workspace.

I think we could run the datafusion-cli tests by adding a new specific CI job that ran them, like:

cd datafusion/datafusion-cli
cargo test --all-features

What do you think @r4ntix ?

@r4ntix
Copy link
Contributor Author

r4ntix commented Jun 8, 2023

@alamb @jiangzhx Thanks.
From #2071 's comment, I see that it can make sense.
I will try to add datafusion-cli test to the CI Job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants