-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Reduce output when sqllogictest runs successfully, and run tests in parallel
#6393
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
Conversation
dcc1f49 to
e4b7ff7
Compare
| run_test_file(&path, relative_path).await?; | ||
| let test_files: Vec<_> = read_test_files(&options).collect(); | ||
|
|
||
| // Run all tests in parallel, reporting failures at the end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering, if there can be a race condition if multiple tests work with table t1, and the table dropped by test who has finished first. Other test could fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each slt file runs with its own SessionContext so I think they should be isolated from one another.
I suppose if they all shared a temporary directory or something else that could be changed, that would be a problem.
Perhaps I can add some comments in various places explaining why it is important to keep the tests from having externally visible side effects so they can be parallelized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
FYI @melgenek and @xudong963 -- I am not sure if you are interested in this PR, but if you had time to review I would be most appreciative |
melgenek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very neat. Thank you!
| .join() | ||
| .unwrap(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main function above for Windows creates a single-threaded executor. I made it this way by mistake.
It should be tokio::runtime::Builder::new_multi_thread().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 19f145c 👍
xudong963
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't find time to do the feature, thanks @alamb making it happen!
Co-authored-by: Yevhenii Melnyk <melnyk.yevhenii@gmail.com>
…datafusion into alamb/sqllogictest_parallel
|
Thanks everyone! |
Which issue does this PR close?
Closes #6392
Closes #4734
Rationale for this change
I want to run sqllogictests faster locally to increase development velocity
What changes are included in this PR?
Are these changes tested?
They are all tested and I tested manually in a few other scenarios:
Timings
On my 8 core laptop:
Main (10s)
This PR (2.5s)
Successful Case
Example output with a single test diff:
Example output with two files that error
Are there any user-facing changes?