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

[task #8213]Port tests in select.rs to sqllogictest #8967

Merged
merged 5 commits into from
Jan 25, 2024

Conversation

Tangruilin
Copy link
Contributor

@Tangruilin Tangruilin commented Jan 23, 2024

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?

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 23, 2024
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @Tangruilin

@Tangruilin
Copy link
Contributor Author

image
for the sql PREPARE my_plan(INT, DOUBLE) AS SELECT c1, c2 FROM test WHERE c1 > $2 AND c1 < $1

@Tangruilin
Copy link
Contributor Author

image for the sql PREPARE my_plan(INT, DOUBLE) AS SELECT c1, c2 FROM test WHERE c1 > $2 AND c1 < $1

There is a error for the sql, and others I have all solved.
You can review it.
@alamb

@comphead
Copy link
Contributor

Tests are failing, and btw there is also another PR #8953

@simicd
Copy link
Contributor

simicd commented Jan 24, 2024

Thanks @comphead! Indeed, we should probably consolidate into one PR, just left a comment here #8213 (comment):

Hi both - @Tangruilin seems like we we're working independently on the topic 😮 I had raised two PRs previously but seems they remained unnoticed. Guess we should try to consolidate into a single PR, feel free to take over whatever you're missing currently from #8953 and #8939 and integrate into yours if that's ok with you as well.

In #8939 you will find how I implemented the structs.

image

@Tangruilin Tangruilin force-pushed the task#8213 branch 2 times, most recently from d048115 to 4b64db8 Compare January 25, 2024 02:39
@Tangruilin
Copy link
Contributor Author

Tests are failing, and btw there is also another PR #8953

I have solved the failed, you can review it again

@Tangruilin Tangruilin force-pushed the task#8213 branch 2 times, most recently from dd3caa6 to de686a3 Compare January 25, 2024 04:13
@Tangruilin
Copy link
Contributor Author

@alamb This CR is ready to be review

Copy link
Contributor

@alamb alamb left a 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;
Copy link
Contributor

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")

@alamb
Copy link
Contributor

alamb commented Jan 25, 2024

I filed #8999 to try and clarify the best practices with contributing

@alamb alamb merged commit fa65c68 into apache:main Jan 25, 2024
22 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 25, 2024

Thanks again everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port tests in select.rs to sqllogictest
4 participants