feat: introduce rust native sqllogictest framework #9150

xudong963 · 2022-12-08T10:43:57Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

The ticket aims to implement rust native sqllogictest framework and replace current python version.

Reasons:

Unify our codebase, all in rust!
The maintainer of the python version has left databend, the python version is a little difficult to maintain.
sqllogictest-rs becomes more and more stable, the guys are talented, and we can work with them to make sqllogictest-rs better

Todos:

refactor code: walking dir to read all test files, remove hard code and etc.
change current logictest file's syntax, make it standard: such as statement query II -> query II.
add args to support run specific test dir or file.
make all tests can be run under new sqllogictest framework.
add it to CI

After finishing the above todos, I'll make it ready for review (maybe the weekend)

Future todos:

See #9174

Closes #issue

vercel · 2022-12-08T10:44:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
databend	⬜️ Ignored (Inspect)		Dec 17, 2022 at 7:20AM (UTC)

Xuanwo · 2022-12-08T10:52:14Z

I like this!

flaneur2020 · 2022-12-08T10:53:03Z

+10086!

tests/sqllogictests/Cargo.toml

tests/sqllogictests/suits/test.slt

tests/sqllogictests/src/error.rs

tests/sqllogictests/Cargo.toml

xudong963 · 2022-12-08T17:06:28Z

0b7305a The commit fixed test files format and is large. So you can ignore it when reviewing.

xudong963 · 2022-12-09T08:37:27Z

Support to run specific dir or test files:

➜  databend git:(new_sqllogictest) ✗ cargo run -p sqllogictests -- --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
     Running `target/debug/sqllogictests --help`
Mysql client starts to run...
Usage: sqllogictests [OPTIONS]

Options:
  -d, --dir <DIR>    Run sqllogictests in specific directory, the arg is optional
  -f, --file <FILE>  Run sqllogictests in specific test file, the arg is optional
  -h, --help         Print help information

Xuanwo · 2022-12-09T09:44:03Z

Support to run specific dir or test files:

sqllogictest has a binary which supports pg, can we share our work with them? I think we only need to maintain the HTTP API part.

xudong963 · 2022-12-09T09:46:20Z

Support to run specific dir or test files:

sqllogictest has a binary which supports pg, can we share our work with them? I think we only need to maintain the HTTP API part.

Do you mean let sqllogictest-rs binary support mysql?

Xuanwo · 2022-12-09T09:47:47Z

Do you mean let sqllogictest-rs binary support mysql?

Yes, we can run the binary directly instead of cargo run which need to build the runner everytime.

xudong963 · 2022-12-09T09:50:56Z

Do you mean let sqllogictest-rs binary support mysql?

Yes, we can run the binary directly instead of cargo run which need to build the runner everytime.

This can be advanced afterwards, perhaps until their next release, my efforts now are mainly focused on getting the new framework running inside the CI.

I opened an issue last night: risinglightdb/sqllogictest-rs#120. Should be part of your proposal

Xuanwo · 2022-12-13T16:51:53Z

Respect! 🙇

Xuanwo · 2022-12-13T16:53:14Z

I think add ability to process Regex can be moved to next PR.

xudong963 · 2022-12-13T16:54:10Z

I think add ability to process Regex can be moved to next PR.

Yes, it's very tricky, I think we should find a more suitable test framework to support it.

xudong963 · 2022-12-13T16:55:28Z

Respect! 🙇

Still fighting! HTTP and ck HTTP client also need to check, I think there are still many blocks.

xudong963 · 2022-12-17T05:06:08Z

It seems like a flaky test, but it can't reproduce in my local... (Btw, it's normal in old logictest)

BohuTANG · 2022-12-17T07:16:37Z

@xudong963

If this one test is blocked for a while, we can file a tracking issue and skip it in this PR.

xudong963

Now the ticket is ready for review! Its changes contain three parts: CI, logictest files and new sqllogictest framework. The following is a summary of the ticket(hope it will help reviewers):

For CI, the main changes are removing some files used by old framework and changing the commands that run new logictests. cc @Xuanwo @everpcpc @PsiACE
For test files, the changes are as follows
- Remove the semicolon after the query.
- Remove the blank line between the query and ----.
- Replace the comment symbol from -- to ##.
- For those results where the output is empty, such as select '', 1; the first cell, will be displayed instead of (empty), which can reduce confusion.
- Temporarily commenting out queries that contain dynamic results in their output, such as db1 t1 FUSE $ANYTHING $DATE NULL 0 0 0 0 0, we need a better way to handle them.
For new sqllogictest framework, it's under sqllogictests dir.
- It's based on sqllogictest-rs crate.
- It supports three clients. ping @youngsofun for review
- Walk dir part needs to review carefully

FYI, the future todos: #9174

BohuTANG · 2022-12-17T08:31:17Z

Let's merge first, since it with a big diff.

Xuanwo · 2022-12-17T08:49:03Z

tests/sqllogictests/src/arg.rs

+        long = "run_dir",
+        help = "Run sqllogictests in specific directory, the arg is optional"
+    )]
+    pub dir: Option<String>,


Maybe we can merge dir and file into a glob path? So that we will have

path: Vec<String> => the glob paths for tests

skip_path: Vec<String> => the gloab paths that need to ignore

./test -p "ydb/select**" -s "ydn/select1-1.test"

Xuanwo · 2022-12-17T08:52:45Z

tests/sqllogictests/src/client/clickhouse_client.rs

+impl ClickhouseHttpClient {
+    pub fn create() -> Result<ClickhouseHttpClient> {
+        let client = Client::new();
+        let url = "http://127.0.0.1:8124".to_string();


There is an existing issue with this: #8689

It's better to support accept a Vec<String> as request URLs and we can call different node in the cluster for better performance and find more potential bug.

Xuanwo · 2022-12-17T08:54:48Z

tests/sqllogictests/src/client/clickhouse_client.rs

+
+    pub async fn query(&mut self, sql: &str) -> Result<DBOutput> {
+        // Client will save the following info: use database, settings (session level info)
+        // Then send them to server, so even though the session changes, database and settings context is correct


This behavior seems weird. After a session has been restored, all session related changes should be reset. Is this the same behavior of a real clickhouse service?

Xuanwo · 2022-12-17T08:56:00Z

tests/sqllogictests/src/client/clickhouse_client.rs

+            .basic_auth("root", Some(""))
+            .send()
+            .await?;
+        // `res` is tsv format


Can we use json as a response?

Xuanwo · 2022-12-17T08:56:46Z

tests/sqllogictests/src/client/http_client.rs

+    pub fn create() -> Result<HttpClient> {
+        let mut header = HeaderMap::new();
+        header.insert(
+            "Content-Type",


It's better to use http::header::CONTENT_TYPE.

Xuanwo · 2022-12-17T09:02:02Z

tests/sqllogictests/src/client/http_client.rs

+        let mut response = self.response(sql, &url, true).await?;
+        // Set session from response to client
+        // Then client will same session for different queries.
+        self.session = serde_json::from_value(


How about read the whole content via response.bytes() and deserialize as json instead of calling response.as_object() multiple times?

Or maybe just response.json()?

Xuanwo · 2022-12-17T09:04:43Z

tests/sqllogictests/src/error.rs

+    SelfError(String),
+}
+
+impl From<TestError> for DSqlLogicTestError {


We can use Disconnect(#[from] io::Error) to avoid those code.

Xuanwo · 2022-12-17T09:05:59Z

tests/sqllogictests/src/main.rs

+            return http_client.query(sql).await;
+        }
+        println!("Running sql with clickhouse client: [{}]", sql);
+        self.ck_client.as_mut().unwrap().query(sql).await


Why we don't need to check this ck_client?

If we must chose on from those three engines, I prefer to use an enum here for better reading.

Xuanwo · 2022-12-17T09:08:19Z

tests/sqllogictests/src/main.rs

+        for handler in handlers.iter() {
+            match handler.as_str() {
+                "mysql" => {
+                    println!("Mysql client starts to run...");


Use log instead of println so users have more control on this.

Xuanwo · 2022-12-17T09:10:39Z

tests/sqllogictests/src/main.rs

+    env_logger::init();
+    let args = SqlLogicTestArgs::parse();
+    if let Some(handlers) = &args.handlers {
+        for handler in handlers.iter() {


Maybe it's simpler to use mysql,http,clickhouse as the default value of handlers?

Xuanwo · 2022-12-20T08:49:45Z

cc @xudong963, here are some review you may be interested.

xudong963 · 2022-12-20T09:14:41Z

cc @xudong963, here are some review you may be interested.

Oh, I forgot it, will take a look later! Thanks @Xuanwo

xudong963 requested review from Xuanwo and PsiACE as code owners December 8, 2022 10:43

xudong963 marked this pull request as draft December 8, 2022 10:44

xudong963 removed request for Xuanwo and PsiACE December 8, 2022 10:44

mergify bot added the pr-feature this PR introduces a new feature to the codebase label Dec 8, 2022

Xuanwo reviewed Dec 8, 2022

View reviewed changes

tests/sqllogictests/Cargo.toml Outdated Show resolved Hide resolved

tests/sqllogictests/suits/test.slt Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

Xuanwo reviewed Dec 8, 2022

View reviewed changes

tests/sqllogictests/src/error.rs Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

PsiACE reviewed Dec 8, 2022

View reviewed changes

tests/sqllogictests/Cargo.toml Show resolved Hide resolved

Xuanwo mentioned this pull request Dec 8, 2022

feat-request: Allow differnet file extentions as input risinglightdb/sqllogictest-rs#118

Closed

vercel bot deployed to Preview December 8, 2022 17:07 View deployment

xudong963 force-pushed the new_sqllogictest branch from 65ebd50 to dd0f83f Compare December 9, 2022 08:41

xudong963 mentioned this pull request Dec 9, 2022

Tracking: enhance sqllogictest framework for databend #9174

Closed

15 tasks

xudong963 mentioned this pull request Dec 13, 2022

chore: clean error and useless tests #9233

Merged

xudong963 force-pushed the new_sqllogictest branch from 5dd0296 to 54b683b Compare December 14, 2022 08:22

fix some tests

c527ed4

xudong963 force-pushed the new_sqllogictest branch from 0710405 to c527ed4 Compare December 16, 2022 12:12

xudong963 added 3 commits December 16, 2022 20:40

use thiserror

8238995

onlyif todo

0e5833d

fix walk dir

9939e6b

xudong963 force-pushed the new_sqllogictest branch from 35494bd to 9939e6b Compare December 16, 2022 16:03

update ci

e0ba386

xudong963 mentioned this pull request Dec 17, 2022

Tracking: enable more test suits from cluster #9270

Closed

change args, handler to handlers

bc7febb

xudong963 force-pushed the new_sqllogictest branch from a2e6a5d to bf70a93 Compare December 17, 2022 07:02

fix ci

a06d81e

xudong963 force-pushed the new_sqllogictest branch from bf70a93 to a06d81e Compare December 17, 2022 07:20

xudong963 commented Dec 17, 2022

View reviewed changes

xudong963 marked this pull request as ready for review December 17, 2022 08:06

xudong963 requested review from leiysky, sundy-li, youngsofun, everpcpc, Xuanwo, PsiACE and BohuTANG December 17, 2022 08:07

BohuTANG approved these changes Dec 17, 2022

View reviewed changes

BohuTANG merged commit 18ef7e3 into databendlabs:main Dec 17, 2022

Xuanwo reviewed Dec 17, 2022

View reviewed changes

xudong963 deleted the new_sqllogictest branch December 20, 2022 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce rust native sqllogictest framework #9150

feat: introduce rust native sqllogictest framework #9150

xudong963 commented Dec 8, 2022 •

edited

Loading

vercel bot commented Dec 8, 2022 •

edited

Loading

Xuanwo commented Dec 8, 2022

flaneur2020 commented Dec 8, 2022

This comment was marked as resolved.

This comment was marked as resolved.

xudong963 commented Dec 8, 2022

xudong963 commented Dec 9, 2022

Xuanwo commented Dec 9, 2022

xudong963 commented Dec 9, 2022

Xuanwo commented Dec 9, 2022

xudong963 commented Dec 9, 2022

Xuanwo commented Dec 13, 2022

Xuanwo commented Dec 13, 2022

xudong963 commented Dec 13, 2022

xudong963 commented Dec 13, 2022

xudong963 commented Dec 17, 2022

BohuTANG commented Dec 17, 2022

xudong963 left a comment •

edited

Loading

BohuTANG commented Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo Dec 17, 2022

Xuanwo commented Dec 20, 2022 •

edited

Loading

xudong963 commented Dec 20, 2022

feat: introduce rust native sqllogictest framework #9150

feat: introduce rust native sqllogictest framework #9150

Conversation

xudong963 commented Dec 8, 2022 • edited Loading

Summary

vercel bot commented Dec 8, 2022 • edited Loading

Xuanwo commented Dec 8, 2022

flaneur2020 commented Dec 8, 2022

This comment was marked as resolved.

This comment was marked as resolved.

xudong963 commented Dec 8, 2022

xudong963 commented Dec 9, 2022

Xuanwo commented Dec 9, 2022

xudong963 commented Dec 9, 2022

Xuanwo commented Dec 9, 2022

xudong963 commented Dec 9, 2022

Xuanwo commented Dec 13, 2022

Xuanwo commented Dec 13, 2022

xudong963 commented Dec 13, 2022

xudong963 commented Dec 13, 2022

xudong963 commented Dec 17, 2022

BohuTANG commented Dec 17, 2022

xudong963 left a comment • edited Loading

Choose a reason for hiding this comment

BohuTANG commented Dec 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Dec 20, 2022 • edited Loading

xudong963 commented Dec 20, 2022

xudong963 commented Dec 8, 2022 •

edited

Loading

vercel bot commented Dec 8, 2022 •

edited

Loading

xudong963 left a comment •

edited

Loading

Xuanwo commented Dec 20, 2022 •

edited

Loading