-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[sqllogictest] port tests in avro.rs to sqllogictest #6362
Conversation
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.
Thank you for the contribution @e1ijah1 -- this is looking very close. I left some comments about how to port the last test. Thank you 🙏
} | ||
|
||
#[tokio::test] | ||
async fn avro_query_multiple_files() { |
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.
I see that this test was not ported directly -- since the avro code uses the same codepath as all the other listing tables for handling multiple files it is probably ok.
However, I think it is important coverage to maintain as the end to end coverage makes sure everything is hooked up correctly.
I think the issue is there is no way to setup these files using sqllogictest. Perhaps you can add a special sqllogictest test setup (to make the directory with multiple files) following this model:
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.
Considering that avro_query_multiple_files
utilizes a temporary directory, could we maintain the temporary directory within SessionContext
to facilitate the creation of necessary data during the setup phase?
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.
🤔 I think adding something to SessionContext
for testing only is likely not the best idea
Maybe we could return the TempDir alongside the SessionContext
like:
struct TestContext {
/// Context for running queries
ctx: SessionContext,
/// Temporary directory created and cleared at the end of the test
tmpdir: TempDir
}
And then change
async fn context_for_test_file(relative_path: &Path) -> SessionContext {
to
async fn context_for_test_file(relative_path: &Path) -> TestContext {
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.
Thank you for taking the time to review my PR 🙏. I have updated the code. Please take another look.
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.
Looks great -- thank you @e1ijah1
This PR was failing on CI: https://github.com/apache/arrow-datafusion/actions/runs/5010297388/jobs/8991828477 -- I pushed some fixes for |
Thanks again @e1ijah1 |
Which issue does this PR close?
Closes #6299
Rationale for this change
#6195
What changes are included in this PR?
Port the Rust unit tests from avro.rs, excluding avro_query_multiple_files, to sqllogictest.
Are these changes tested?
yes.
Are there any user-facing changes?