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

Move some create/drop tests to ddl.slt #4535

Merged
merged 3 commits into from
Dec 9, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 6, 2022

Draft as it builds on #4530

Which issue does this PR close?

Part of #4495

Rationale for this change

In #4495 we are porting tests from rust to sqllogictest for speed and consistency

What changes are included in this PR?

Port some rust based tests in create_drop.rs to sqllogictests

There are a few tests I can't really port at the moment as they require programatic manipulation of the SessionContext (both information schema as well as table factories).

Are these changes tested?

Yes

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Dec 6, 2022
@xudong963 xudong963 self-requested a review December 7, 2022 07:05
@alamb alamb force-pushed the alamb/port_create_drop branch from 324557d to 1ffe19b Compare December 7, 2022 19:42
@github-actions github-actions bot removed logical-expr Logical plan and expressions sql SQL Planner labels Dec 7, 2022
@alamb alamb force-pushed the alamb/port_create_drop branch from 1ffe19b to 9917a6c Compare December 7, 2022 20:11
Copy link
Contributor Author

@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.

@@ -0,0 +1,2 @@
Jorge,2018-12-13T12:12:10.011Z
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These files used to be generated programatically in Rust, instead I have checked them in for the tests

@@ -182,5 +182,234 @@ DROP VIEW foo, bar;
statement ok
DROP VIEW foo;

# Ok to drop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the SLT files are much easier to add / work with ❤️

}

#[tokio::test]
#[should_panic(expected = "doesn't exist")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One difference is that .slt files don't seem to offer a way to expect a certain error message (seemingly by design). I am not sure if you have any thoughts / experience on this @xudong963

Copy link
Member

Choose a reason for hiding this comment

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

I think it can be supported in sqllogictest-rs.

Currently, databend has written its own python version of the logictest framework, which is able to add error messages, such as statement error 1006: 1006 is errorcode.

So I think it's a reasonable feature ~ ping @xxchan.

Copy link
Member

Choose a reason for hiding this comment

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

We do support error messages:

statement error <regexp> and query error <regexp>

example can be found here https://github.com/risinglightdb/sqllogictest-rs/blob/main/examples/basic/basic.slt#L20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet -- hank you @xxchan I will update these tests to use that syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the expected messages in 6be3662 -- very cool! It was good enough that I see a few bugs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is an improvement to fix the messages: #4572

@alamb alamb marked this pull request as ready for review December 7, 2022 20:15
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

LGTM, the notes in the test are great too, @alamb you are as meticulous as ever 🧑‍🍳 !

@alamb alamb merged commit 63ec268 into apache:master Dec 9, 2022
@alamb alamb deleted the alamb/port_create_drop branch December 9, 2022 18:33
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.

3 participants