Skip to content

Comments

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

Merged
alamb merged 5 commits intoapache:mainfrom
Tangruilin:task#8213
Jan 25, 2024
Merged

[task #8213]Port tests in select.rs to sqllogictest#8967
alamb merged 5 commits intoapache:mainfrom
Tangruilin:task#8213

Conversation

@Tangruilin
Copy link
Contributor

@Tangruilin Tangruilin commented Jan 23, 2024 •
edited by alamb
Loading

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
comphead
comphead approved these changes Jan 23, 2024
View reviewed changes
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

Tangruilin commented Jan 24, 2024

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

Tangruilin commented Jan 24, 2024

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

@Tangruilin Tangruilin force-pushed the task#8213 branch 2 times, most recently from 6246f08 to fbf0604 Compare January 24, 2024 10:59
@alamb alamb mentioned this pull request Jan 24, 2024
Closed
9 tasks
@comphead
Copy link
Contributor

comphead commented Jan 24, 2024

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

Tangruilin commented Jan 25, 2024

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 Tangruilin requested a review from comphead January 25, 2024 05:18
@Tangruilin
Copy link
Contributor Author

Tangruilin commented Jan 25, 2024

@alamb This CR is ready to be review

alamb
alamb approved these changes Jan 25, 2024
View reviewed changes
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

datafusion/sqllogictest/test_files/select.slt Outdated

# test_list_query_parameters
query IIB
SELECT * FROM query_parameters_table WHERE c1 = 3;
Copy link
Contributor

@alamb alamb Jan 25, 2024

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 alamb mentioned this pull request Jan 25, 2024
Merged
@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
@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

Reviewers

@alamb alamb alamb approved these changes

@comphead comphead Awaiting requested review from comphead

Assignees

No one assigned

Labels

core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Port tests in select.rs to sqllogictest

4 participants

@Tangruilin @comphead @simicd @alamb