-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[task #8213]Port tests in select.rs to sqllogictest #8967
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.
lgtm thanks @Tangruilin
There is a error for the sql, and others I have all solved. |
6246f08
to
fbf0604
Compare
Tests are failing, and btw there is also another PR #8953 |
Thanks @comphead! Indeed, we should probably consolidate into one PR, just left a comment here #8213 (comment):
|
d048115
to
4b64db8
Compare
I have solved the failed, you can review it again |
dd3caa6
to
de686a3
Compare
Signed-off-by: tangruilin <[email protected]>
@alamb This CR is ready to be review |
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 @Tangruilin and @simicd -- I went through this carefully and the only issue I found was that the query parameter test I think can't be written in SQL so needs to part of .rs
Since this PR has dragged out a bunch already, I made the adjustment myself and I plan to merge this PR once the CI passes.
I am really sorry @simicd that we didn't see / prevent this from being implemented in parallel to #8953 and #8939
I will make a PR to the contributor guide to try and make the expectations clearer and hopefully avoid this kind of repetition in the future
|
||
# test_list_query_parameters | ||
query IIB | ||
SELECT * FROM query_parameters_table WHERE c1 = 3; |
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 this is different than the original test where the parameter is filled in via with_param_value
. I will restore it back to the .rs file
.sql("SELECT * FROM test WHERE c1 = $1")
I filed #8999 to try and clarify the best practices with contributing |
Thanks again everyone |
Which issue does this PR close?
Closes #8213
Closes #8939
Closes #8953
.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?