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

Minor: Improve error handling in sqllogictest runner #8544

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 14, 2023

Which issue does this PR close?

Closes #8541 (I think)

Rationale for this change

@andygrove got an error when running sqllogictest, which I think was something related to his system setup (e.g. there was some filesystem error) but the error message was unhelpful

What changes are included in this PR?

  1. Added error handling rather than panic

Before this PR

We get a somewhat unhelpful panic message about "Readable directory"

andrewlamb@Andrews-MacBook-Pro:~/Software/arrow-datafusion$ cargo test --test sqllogictests
    Finished test [unoptimized + debuginfo] target(s) in 0.13s
     Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-a4acaa02cc91a4e3)
thread 'main' panicked at datafusion/sqllogictest/bin/sqllogictests.rs:264:14:
Readable directory: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`

After this PR

The error I think is quite a but clearer

andrewlamb@Andrews-MacBook-Pro:~/Software/arrow-datafusion$ cargo test --test sqllogictests
   Compiling datafusion-sqllogictest v34.0.0 (/Users/andrewlamb/Software/arrow-datafusion/datafusion/sqllogictest)
    Finished test [unoptimized + debuginfo] target(s) in 0.94s
     Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-a4acaa02cc91a4e3)
Error: Execution("Error reading directory \"test_files/\": No such file or directory (os error 2)")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`

Caused by:
  process didn't exit successfully: `/Users/andrewlamb/Software/arrow-datafusion/target/debug/deps/sqllogictests-a4acaa02cc91a4e3` (exit status: 1)

Are these changes tested?

Are there any user-facing changes?

@alamb alamb marked this pull request as ready for review December 14, 2023 17:05
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Dec 14, 2023
fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item = PathBuf>> {
Box::new(
std::fs::read_dir(path)
.expect("Readable directory")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of this PR is to remove the expect in this line and the line below

While I am sure we could retain the iterator nature of the previous implementation, given we are talking about less than 30 files I think buffering them in a Vec is perfectly acceptable and makes the error handling easier

Copy link
Contributor

@metesynnada metesynnada left a comment

Choose a reason for hiding this comment

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

I think we can merge this, LGTM. It will indeed improve error messages.

@andygrove
Copy link
Member

I tried this out, but of course, I can no longer reproduce the issue 😞

@andygrove andygrove merged commit efa7b34 into apache:main Dec 14, 2023
22 checks passed
@alamb alamb deleted the alamb/better_errors branch December 14, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sqllogictests failing locally due to "No such file or directory"
3 participants