-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[sqllogictest] Define output types and check them in tests #5253
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
datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs
Outdated
Show resolved
Hide resolved
datafusion/core/tests/sqllogictests/test_files/pg_compat/pg_compat_type_coercion.slt
Show resolved
Hide resolved
alamb
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.
Thank you so much @melgenek this is looking great. This feature is so cool!
Prior to merging this PR, I would like to suggest:
- Update the type characters to be more inline with other systems
- Wait for a release of
sqllogictests-rsthat includes the complete feature (as I think it is critical to the maintenance of the sqllogictests in datafusion)
cc @xudong963
datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs
Outdated
Show resolved
Hide resolved
|
Cool, I'm traveling. Will review it as soon as possible. |
👍 |
alamb
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.
LGTM -- thanks @melgenek -- no we just need to wait on the sqllogictest release / upgrade and we'll be good to go
|
Marking as draft until the upstream release is ready |
# Conflicts: # datafusion/core/tests/sqllogictests/src/engines/datafusion/normalize.rs # datafusion/core/tests/sqllogictests/test_files/select.slt
|
@alamb I updated the This pr is good to go. |
alamb
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.
Thank you so much @melgenek
I merged this PR up to the latest main and ran cargo test --test sqllogictests -- --complete and there are no changes 👍
Then I also tried purposely updating test output to be incorrect, and I got an error as expected
Error: query columns mismatch:
[SQL] SELECT * from information_schema.tables;
[Expected] [R][R]
[Actual ] [T][T][T][T]
at tests/sqllogictests/test_files/information_schema.slt:33
And then cargo test --test sqllogictests -- --complete fixed the problem I introduced
❤️ 🚀 -- thank you
|
Benchmark runs are scheduled for baseline = f154a9a and contender = 22b974f. 22b974f is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* [sqllogictest] Define output types and check them in tests * Don't change imports * Fix import * Check types when results are empty * Use Sqllite compatible types 'T','R','I' * Update sqllogictest-rs to 0.13.0 * Complete types
Which issue does this PR close?
Closes #4499.
Rationale for this change
Currently, the type strings in sqllogictests are ignored, and letters have no meaning to them. Also, some tests benefit, like null checks, benefit from type checking.
What changes are included in this PR?
DFColumnTypethat has more types than the original sqllite set of typesAre these changes tested?
Github actions.
Are there any user-facing changes?
No