Add sqllogictests (v0) #4395

mvanschellebeeck · 2022-11-27T21:25:26Z

Initial PR to setup sqllogictests - #4248

xudong963 · 2022-11-28T05:28:11Z

Thanks!!! @mvanschellebeeck

I'll review it carefully later!

liurenjie1024 · 2022-11-28T05:53:51Z

Thanks @mvanschellebeeck I will also help to review this when it's ready from review.

martin-g · 2022-11-28T07:03:43Z

tests/sqllogictests/README.md

+
+> :warning: **Warning**:Datafusion's sqllogictest implementation and migration is still in progress. Definitions taken from https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki
+
+sqllogictest is a program originally written for SQLite to verify the correctness of SQL queries against the SQLLite engine. The program is engine-agnostic and can parse sqllogictest files (`.slt`), runs queries against an SQL engine and compare the output to the expected output.


Suggested change

sqllogictest is a program originally written for SQLite to verify the correctness of SQL queries against the SQLLite engine. The program is engine-agnostic and can parse sqllogictest files (`.slt`), runs queries against an SQL engine and compare the output to the expected output.

sqllogictest is a program originally written for SQLite to verify the correctness of SQL queries against the SQLite engine. The program is engine-agnostic and can parse sqllogictest files (`.slt`), runs queries against an SQL engine and compare the output to the expected output.

martin-g · 2022-11-28T07:06:53Z

tests/sqllogictests/README.md

+
+- `test_name`: Uniquely identify the test name (arrow-datafusion only)
+- `type_string`: A short string that specifies the number of result columns and the expected datatype of each result column. There is one character in the <type_string> for each result column. The characters codes are "T" for a text result, "I" for an integer result, and "R" for a floating-point result.
+- (Optional) `label`: sqllogictest stores a hash of the results of this query under the given label. If the label is reused, then sqllogictest verifies that the results are the same. This can be used to verify that two or more queries in the same test script that are logically equivalent always generate the same output. 


There is no explanation for sort_mode

alamb · 2022-11-28T15:48:26Z

I plan to review this later today

alamb · 2022-11-28T22:14:34Z

Thank you so much @mvanschellebeeck -- this looks so cool. I ran out of time today to thoroughly review it, but the code I looked at looks good. I want to try running the harness locally . So exciting!

alamb

Thank you @mvanschellebeeck -- I took this PR for a spin and it an awesome step forward 👍 I left a few comments and I think we could merge it and iterate on master or else fix it in this PR as well.

Thank you @xudong963 for suggesting slqlogictest and @TennyZhuang for the great library 🙏

Next steps

If others agree, I think we should merge this PR and then we can improve / consolidate as individual follow on projects that i think we could split up (I can file a tracking ticket)

Specifically:

Port existing sql_integration tests
Try and find / leverage existing .sql files
Implement "test script completion mode" (which helps updating these scripts)

For "test script completion mode"
https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki

The sqllogictest program operates in two modes: test script completion mode and test script validation mode. In test script completion mode, the sqllogictest program reads a prototype script and runs the statements and queries against a reference database engine. The output is a full script that is a copy of the prototype script with result inserted. In validation mode, the sqllogictest program reads a full script and runs the statements and queries contained therein against a database engine under test. The results received back from the database engine are compared against the results in the full script to validate the output of the database engine.

Testing notes:

I tested purposely introducing a diff and I got a good output

cd /Users/alamb/Software/arrow-datafusion2 && RUST_BACKTRACE=1 CARGO_TARGET_DIR=/Users/alamb/Software/target-df2 cargo run -p datafusion-sqllogictests
    Finished dev [unoptimized + debuginfo] target(s) in 0.29s
     Running `/Users/alamb/Software/target-df2/debug/datafusion-sqllogictests`
[Aggregate] Registering tables
[Aggregate] Running query: "SELECT avg(c12) FROM aggregate_test_100"
[Aggregate] Running query: "SELECT covar_pop(c2, c12) FROM aggregate_test_100"
[Aggregate] Running query: "SELECT covar(c2, c12) FROM aggregate_test_100"
[Aggregate] Running query: "SELECT corr(c2, c12) FROM aggregate_test_100"
[Aggregate] Running query: "SELECT var_pop(c2) FROM aggregate_test_100"
[Aggregate] Running query: "SELECT var_pop(c6) FROM aggregate_test_100"
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: query result mismatch:
[SQL] SELECT var_pop(c6) FROM aggregate_test_100
[Diff]
5.6156334342
2.615633434202189e37

alamb · 2022-11-29T17:26:51Z

Cargo.toml

@@ -31,6 +31,7 @@ members = [
    "test-utils",
    "parquet-test-utils",
    "benchmarks",
+    "tests/sqllogictests",


I recommend moving this test into datafusion/core/tests so that it would then be run via

cargo test -p datafusion --test sqllogictests

I don't see any reason to put it into its own top level crate (though if others feel differently perhaps we could move the code into datafusion/sqllogictest to match the structure of the other crates in this repo.

Well TIL about cargo tests' harness = false! Thanks for the tip

alamb · 2022-11-29T17:28:26Z

tests/sqllogictests/src/main.rs

+        let result = run_query(&self.ctx, sql).await?;
+        Ok(result)
+    }
+}


Suggested change

}

/// Engine name of current database.

fn engine_name(&self) -> &str {

"DataFusion"

}

/// [`Runner`] calls this function to perform sleep.

///

/// The default implementation is `std::thread::sleep`, which is universial to any async runtime

/// but would block the current thread. If you are running in tokio runtime, you should override

/// this by `tokio::time::sleep`.

async fn sleep(dur: Duration) {

tokio::time::sleep(dur).await;

}

}

alamb · 2022-11-29T17:31:31Z

tests/sqllogictests/src/main.rs

+fn format_batches(batches: &[RecordBatch]) -> Result<String> {
+    let mut bytes = vec![];
+    {
+        let builder = WriterBuilder::new().has_headers(false).with_delimiter(b',');


is the reason to write out CSV output so that we can reuse existing slt files?

nope I actually strip the comma later down in the function - I'll set the delimiter to space here and remove the replace call down below

alamb · 2022-11-29T17:42:40Z

tests/sqllogictests/src/main.rs

+
+        let mut tester = sqllogictest::Runner::new(DataFusion { ctx, test_category });
+        // TODO: use tester.run_parallel_async()
+        tester.run_file_async(filename).await.unwrap();


Suggested change

tester.run_file_async(filename).await.unwrap();

tester.run_file_async(filename).await?;

alamb · 2022-11-29T17:45:33Z

tests/sqllogictests/README.md

+
+> :warning: **Warning**:Datafusion's sqllogictest implementation and migration is still in progress. Definitions taken from https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki
+
+sqllogictest is a program originally written for SQLite to verify the correctness of SQL queries against the SQLLite engine. The program is engine-agnostic and can parse sqllogictest files (`.slt`), runs queries against an SQL engine and compare the output to the expected output.


BTW this is an amazing writeup -- thank you -- I recommend we eventually move this content into the sqllogictest repo and link to that document here

Yep makes sense! I'll track this in a later PR as I improve these docs iteratively.

alamb · 2022-11-29T18:11:52Z

BTW CI is failing because the apache license is needed in a few files

Run ./dev/release/run-rat.sh .
NOT APPROVED: tests/sqllogictests/Cargo.toml (./tests/sqllogictests/Cargo.toml): false
NOT APPROVED: tests/sqllogictests/README.md (./tests/sqllogictests/README.md): false
NOT APPROVED: tests/sqllogictests/test_files/aggregate.slt (./tests/sqllogictests/test_files/aggregate.slt): false
NOT APPROVED: tests/sqllogictests/test_files/arrow_typeof.slt (./tests/sqllogictests/test_files/arrow_typeof.slt): false
4 unapproved licences. Check rat report: rat.txt
Error: Process completed with exit code 1.

xudong963 · 2022-11-30T01:01:10Z

Thanks for reviewing @alamb . I'll review it in the evening. (GMT+8)

xudong963

NOTICE.txt and LICENSE.txt should be reserved?

xudong963 · 2022-11-30T13:13:44Z

datafusion/core/tests/sqllogictests/README.md

+- `type_string`: A short string that specifies the number of result columns and the expected datatype of each result column. There is one character in the <type_string> for each result column. The characters codes are "T" for a text result, "I" for an integer result, and "R" for a floating-point result.
+- (Optional) `label`: sqllogictest stores a hash of the results of this query under the given label. If the label is reused, then sqllogictest verifies that the results are the same. This can be used to verify that two or more queries in the same test script that are logically equivalent always generate the same output.
+- `expected_result`: In the results section, integer values are rendered as if by printf("%d"). Floating point values are rendered as if by printf("%.3f"). NULL values are rendered as "NULL". Empty strings are rendered as "(empty)". Within non-empty strings, all control characters and unprintable characters are rendered as "@".
+- `sort_mode`: If included, it must be one of "nosort", "rowsort", or "valuesort". The default is "nosort". In nosort mode, the results appear in exactly the order in which they were received from the database engine. The nosort mode should only be used on queries that have an ORDER BY clause or which only have a single row of result, since otherwise the order of results is undefined and might vary from one database engine to another. The "rowsort" mode gathers all output from the database engine then sorts it by rows on the client side. Sort comparisons use strcmp() on the rendered ASCII text representation of the values. Hence, "9" sorts after "10", not before. The "valuesort" mode works like rowsort except that it does not honor row groupings. Each individual result value is sorted on its own.


Very useful argument to avoid flaky tests 🤣

xudong963 · 2022-11-30T13:14:33Z

datafusion/core/tests/sqllogictests/README.md

+  under the License.
+-->
+
+#### Overview


Very detailed! 👍

xudong963 · 2022-11-30T13:22:45Z

datafusion/core/tests/sqllogictests/src/utils.rs

+};
+use std::sync::Arc;
+
+// TODO: move this to datafusion::test_utils?


mvanschellebeeck · 2022-11-30T23:32:54Z

Thanks for all the reviews @alamb, @xudong963 , @martin-g - I'll merge into master and iterate in future PRs so it becomes easier to follow progress.

mvanschellebeeck · 2022-12-01T00:41:50Z

Hey @alamb, I moved the tests into datafusion/core/tests but the tests were not passing on windows (see CI) so I excluded the tests from running on windows in this commit.

mvanschellebeeck · 2022-12-01T02:41:33Z

Blocked on #4448

xudong963 · 2022-12-01T03:16:43Z

Blocked on #4448

Merged, please update the PR.

liurenjie1024

Nice work!

liurenjie1024 · 2022-12-01T11:09:24Z

datafusion/core/tests/sqllogictests/src/main.rs

+        }
+    }
+
+    async fn register_test_tables(&self, ctx: &SessionContext) {


We need this because currently datafusion doesn't support create table as statement?

xudong963 · 2022-12-01T13:28:24Z

Let's merge it and iterate it step by step! -- Thanks again @mvanschellebeeck

ursabot · 2022-12-01T13:32:27Z

Benchmark runs are scheduled for baseline = 799dd74 and contender = 78ac53a. 78ac53a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

sqllogictests v0

a47e7d9

mvanschellebeeck marked this pull request as draft November 27, 2022 21:25

mvanschellebeeck added 5 commits November 27, 2022 16:30

Add CI workflow

e01653e

Add license

3d654b0

Run linter + remove submodule requirement in CI

1c01f92

Add submodules back

a737c2f

Remove files

96bd42e

mvanschellebeeck mentioned this pull request Nov 27, 2022

Make a data driven SQL testing tool (so we can reuse duckdb test suite, example) #4248

Closed

mvanschellebeeck changed the title ~~sqllogictests v0~~ Add sqllogictests (v0) Nov 27, 2022

martin-g reviewed Nov 28, 2022

View reviewed changes

alamb approved these changes Nov 29, 2022

View reviewed changes

mvanschellebeeck added 2 commits November 29, 2022 18:53

Address comments:

f0d7fef

Move sqllogic tests in datafusion/core/tests

7265e46

github-actions bot added the core Core DataFusion crate label Nov 30, 2022

mvanschellebeeck added 3 commits November 29, 2022 19:21

Update README

77429bb

Add licences

7c93b38

Update CI check

67f9ed4

mvanschellebeeck force-pushed the master branch from fb7e693 to 67f9ed4 Compare November 30, 2022 00:27

rust_lint.sh

6a682aa

mvanschellebeeck marked this pull request as ready for review November 30, 2022 00:29

Run prettier on readme

d04e9b1

mvanschellebeeck added 3 commits November 29, 2022 21:04

Fix checks

fc13102

Merge branch 'master' into master

c3263f0

New line (windows)

941c55b

xudong963 approved these changes Nov 30, 2022

View reviewed changes

sqllogictests don't parse correctly on windows - ignore windows

aee6c62

empty commit - rerun CI

3e7db70

mvanschellebeeck force-pushed the master branch from 65fcbe9 to 3e7db70 Compare December 1, 2022 00:19

Add LICENSE.txt and NOTICE.txt back:

93e58c6

liurenjie1024 approved these changes Dec 1, 2022

View reviewed changes

pyarrow fix

2038db3

xudong963 merged commit 78ac53a into apache:master Dec 1, 2022

This was referenced Dec 1, 2022

[Epic] Data Driven Tests #4460

Closed

Replace python based integration test with sqllogictest #4462

Closed

MINOR: Add note about sqllogictest to contributor guide #4469

Merged

Remove tests from sql_integration that were ported to sqllogictest #4498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sqllogictests (v0) #4395

Add sqllogictests (v0) #4395

mvanschellebeeck commented Nov 27, 2022

xudong963 commented Nov 28, 2022

liurenjie1024 commented Nov 28, 2022

martin-g Nov 28, 2022

martin-g Nov 28, 2022

alamb commented Nov 28, 2022

alamb commented Nov 28, 2022

alamb left a comment

alamb Nov 29, 2022

mvanschellebeeck Nov 30, 2022

alamb Nov 29, 2022

alamb Nov 29, 2022

mvanschellebeeck Nov 29, 2022

alamb Nov 29, 2022

alamb Nov 29, 2022

mvanschellebeeck Nov 30, 2022

alamb commented Nov 29, 2022

xudong963 commented Nov 30, 2022

xudong963 left a comment

xudong963 Nov 30, 2022

xudong963 Nov 30, 2022

xudong963 Nov 30, 2022

mvanschellebeeck commented Nov 30, 2022

mvanschellebeeck commented Dec 1, 2022

mvanschellebeeck commented Dec 1, 2022

xudong963 commented Dec 1, 2022

liurenjie1024 left a comment

liurenjie1024 Dec 1, 2022

xudong963 Dec 1, 2022

xudong963 commented Dec 1, 2022

ursabot commented Dec 1, 2022


		> :warning: Warning:Datafusion's sqllogictest implementation and migration is still in progress. Definitions taken from https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki

		sqllogictest is a program originally written for SQLite to verify the correctness of SQL queries against the SQLLite engine. The program is engine-agnostic and can parse sqllogictest files (`.slt`), runs queries against an SQL engine and compare the output to the expected output.

-}
+    /// Engine name of current database.
+    fn engine_name(&self) -> &str {
+        "DataFusion"
+    }
+    /// [`Runner`] calls this function to perform sleep.
+    ///
+    /// The default implementation is `std::thread::sleep`, which is universial to any async runtime
+    /// but would block the current thread. If you are running in tokio runtime, you should override
+    /// this by `tokio::time::sleep`.
+    async fn sleep(dur: Duration) {
+       tokio::time::sleep(dur).await;
+    }
+}

	tester.run_file_async(filename).await.unwrap();
	tester.run_file_async(filename).await?;

Add sqllogictests (v0) #4395

Add sqllogictests (v0) #4395

Conversation

mvanschellebeeck commented Nov 27, 2022

xudong963 commented Nov 28, 2022

liurenjie1024 commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 28, 2022

alamb commented Nov 28, 2022

alamb left a comment

Choose a reason for hiding this comment

Next steps

Testing notes:

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

alamb commented Nov 29, 2022

xudong963 commented Nov 30, 2022

xudong963 left a 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

mvanschellebeeck commented Nov 30, 2022

mvanschellebeeck commented Dec 1, 2022

mvanschellebeeck commented Dec 1, 2022

xudong963 commented Dec 1, 2022

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 commented Dec 1, 2022

ursabot commented Dec 1, 2022