[DISCUSSION] More SqlLogicTest test coverage for queries, including join queries

@alamb

During SDF's upgrade to DataFusion 43 we found a bug #13425 . This was possible thanks to extensive test coverage for certain query shapes that we have internally, which is good. The bad part is that the bug could be caught at the PR stage, or before the release, should similar tests exist in DataFusion project.

Would it be useful if there were more SLT query tests, including the ones that allowed to catch #13425?

cc @alamb @schulte-lukas @gliga

Yes, 100% I think adding additional test coverage in all areas would be super helpful

We / I also regularly hit bugs that are caught by InfluxData's test suite during upgrades of DataFusion but were not caught during the DataFusion PR

Other Areas that would benefit from improved testing from my experience:

Metadata handling (like attaching field level metadata, etc)
UNION coverage (which we use extensively)
Dictionary handling (which we also use a lot)

One way to extend join coverage is to have some more "answer checking" for different benchmarks, such as:

Verify TPC-DS answers #13073

(Earlier doing this for TPC-H found many issues in joins handling, decimal issues, etc.)

We can do the same for the imdb join order benchmark, clickbench, etc.

I have been looking over the sqllogictests this weekend and I think first of all that they need a good reorganization to clean them up, split them into smaller chunks testing just specific portions of functionality and to reduce the size of some of the files.

I also have started looking at other projects's tests to see how they've done their testing. I am unsure about actually incorporating some of those tests directly into DF (duckdb for example, MIT license vs Apache) but they could be used as a basis for seeing where DF's test suite is falling short and expanding the test suite some more.

I came across this paper which I found interesting for join testing. It won't work for DF yet since DF doesn't support query hints (at least as best as I know).

I would also love to see a separate test-runner (maybe nightly) of the official SQLLite SLTs (where applicable).

At this point DF seems to be able to pass the majority without modification (except delete). Since they are (a) pre-vetted by sqllite and (b) test foundational capability it would make sense to me that they are included in datafusion

I would also love to see a separate test-runner (maybe nightly) of the official SQLLite SLTs (where applicable).

At this point DF seems to be able to pass the majority without modification (except delete). Since they are (a) pre-vetted by sqllite and (b) test foundational capability it would make sense to me that they are included in datafusion

I actually am part way through porting those slt's to something that df's slt test runner can run

Can't wait to see what happens here

We / I also regularly hit bugs that are caught by InfluxData's test suite during upgrades of DataFusion but were not caught during the DataFusion PR

I'm wondering what are test strategies in Influx? is it something we can reuse in DF as well to avoid regression

I'm wondering what are test strategies in Influx? is it something we can reuse in DF as well to avoid regression

We have a bunch of our own basic tests for end to end functionality in our system (some is SQL based, some is "InfluxQL" based)

We have often seen issues where features we use heavily (metadata, and UnionExec, for example) are not as heavily covered in the existing DataFusion test suites 🤔

I'm wondering what are test strategies in Influx? is it something we can reuse in DF as well to avoid regression

We have a bunch of our own basic tests for end to end functionality in our system (some is SQL based, some is "InfluxQL" based)

We have often seen issues where features we use heavily (metadata, and UnionExec, for example) are not as heavily covered in the existing DataFusion test suites 🤔

is it real world use cases ?

if its not I found something used for MariaDB and get investigate it https://mariadb.com/kb/en/random-query-generator-tests/

@2010YOUY01

After the slt work I was thinking of taking a look at the work that @2010YOUY01 has done with sqlancer to see if there are areas that I could add into it. I noticed that the datafusion-contrib for that was merged into sqlancer main a few months back - https://github.com/sqlancer/sqlancer/tree/main/src/sqlancer/datafusion.

sqlancer has quite a few options wrt test oracles that might be useful to look at. The one I was looking at (DQP) I think would need support for query hints (or corresponding set options) to function but if we could get it to work would allow a lot of join testing to happen.

I agree -- I think using sqlancer is a great idea -- I suspect it simply needs someone to maintain and file tickets

I would also love to see a separate test-runner (maybe nightly) of the official SQLLite SLTs (where applicable).
At this point DF seems to be able to pass the majority without modification (except delete). Since they are (a) pre-vetted by sqllite and (b) test foundational capability it would make sense to me that they are included in datafusion

I actually am part way through porting those slt's to something that df's slt test runner can run

I've gotten many of them to run so far after removing or changing troublesome features (create index, valuesort, etc). There are differences with sqllite vs DataFusion wrt how sqllite handles integers vs real numbers that I'm still working through slowly. I do see a number of failures that I believe are real or at least point to limitations of DF. I'm hoping to have a full 'clean' run sometime in the next week that only fails on likely valid queries.

Here is an example of something that is currently failing:

CREATE TABLE tab0(pk INTEGER PRIMARY KEY, col0 INTEGER, col1 FLOAT, col2 TEXT, col3 INTEGER, col4 FLOAT, col5 TEXT);
INSERT INTO tab0 VALUES(0,1164,1149.64,'eyxra',1147,1174.8,'fpigl');
INSERT INTO tab0 VALUES(1,1165,1150.72,'zqxbf',1148,1175.52,'zmukh');
INSERT INTO tab0 VALUES(2,1166,1154.12,'itglg',1149,1176.1,'cfmkm');
INSERT INTO tab0 VALUES(3,1167,1156.44,'xmaca',1150,1177.85,'ggpiv');
INSERT INTO tab0 VALUES(4,1168,1157.98,'lewrx',1151,1178.59,'hjscv');
INSERT INTO tab0 VALUES(5,1169,1158.81,'enubf',1152,1180.13,'apmjh');
INSERT INTO tab0 VALUES(6,1171,1159.96,'olnov',1153,1181.89,'smeyx');
INSERT INTO tab0 VALUES(7,1172,1160.51,'yngfz',1154,1182.90,'vakuy');
INSERT INTO tab0 VALUES(8,1174,1161.72,'awjqq',1155,1183.93,'qzqaw');
SELECT pk, col0 FROM tab0 WHERE col0 IN (SELECT col3 FROM tab0 WHERE col4 > 73.18) ORDER BY 2 DESC;

SanityCheckPlan
caused by
Error during planning: Plan: ["SortPreservingMergeExec: [col0@1 DESC]", "  CoalesceBatchesExec: target_batch_size=8192", "    HashJoinExec: mode=Partitioned, join_type=RightSemi, on=[(col3@0, col0@1)]", "      CoalesceBatchesExec: target_batch_size=8192", "        RepartitionExec: partitioning=Hash([col3@0], 20), input_partitions=20", "          RepartitionExec: partitioning=RoundRobinBatch(20), input_partitions=1", "            CoalesceBatchesExec: target_batch_size=8192", "              FilterExec: CAST(col4@1 AS Float64) > 73.18, projection=[col3@0]", "                MemoryExec: partitions=1, partition_sizes=[9]", "      SortExec: expr=[pk@0 DESC], preserve_partitioning=[true]", "        CoalesceBatchesExec: target_batch_size=8192", "          RepartitionExec: partitioning=Hash([col0@1], 20), input_partitions=1", "            MemoryExec: partitions=1, partition_sizes=[9]"] does not satisfy order requirements: [col0@1 DESC]. Child-0 order: [[pk@0 DESC]]

It would be great if there was a common infrastructure across
databases for slt. High level shown below.

------------     --------------   ----------------
|  DF slt  |     | SQLite slt |   | whatever slt |
------------     --------------   ----------------
    |                 |                 |
 ----------------------------------------------    --------------
 |     slt transpiler                         |<---| slt bank   |
 ----------------------------------------------    --------------
    |                 |                 |
    v                 v                 v
------------     --------------   ----------------
|  DF slt  |     | SQLite slt |   | whatever slt |
------------     --------------   ----------------

Boxes on the top are existing slt tests available in various
databases/repositories, e.g., SQLite (which is mostly discussed in
this thread).

slt bank is a set of slt tests independent of any dialect (or even
potentially dialect specific) that is contributed by third party
groups, e.g., researchers.

slt transpiler takes existing slt tests and transpiles to the
desired target (e.g., DF). Translation steps could be:

Take one record at a time
Transpile SQL to the target dialect (using one of the existing tools)
Adjust output (likely most challenging)
Output record: if it works keep it if it does not comment out in the output

The transpiler would also be a repository of links to existing slts.

Boxes at the bottom are the resulting slt tests.

Ideally, the transpilation would not be a one off process, but it
would be done for example every time slt tests are run. Benefit of
such an approach would be to ensure that any updates made to existing
tests (e.g., new tests added to SQLite) are reflected in the target
run (e.g., DF runs).

@Omega359

@Omega359 I see you went quite far and already had some good findings.
Would you need any help at this point?

@gliga

The result is that everything runs however a LOT of queries are skipped for DF for various reasons.

that's totally understandable! what about having them with query error msg instead of skip, so that they act as "executable todo" and thus we don't need to remember to find and enable tests when corresponding functionality is fixed?

I was torn about this because it would mean that the expected query types and results would then be lost.

Because of the format differences between the slt files in DF vs sqlite a custom version of sqllogictest dependency is required.

i know you mentioned this earlier (#13470 (comment)), but i unfortunately don't know what the difference is. How big it is? Can we avoid custom dependency with eg a regexp replace? cc @gliga

The primary issue is that those slt files are in what I call 'valuewise' ordering, meaning that results are in a single column. For example:

query II rowsort
SELECT ALL + col1 AS col0, col2 AS col2 FROM tab1
----
14
96
47
68
5
59

Every other use of slt's use what I call 'rowwise' where the results are in proper columns. Both have their uses - valuewise better handles string with leading/trailing spaces, rowwise is easier to parse.

The main change I made to the sltlogictest-rs dep is to add support for control resultmode valuewise which you will see at the top of all the sqlite test files to allow that lib to be able to compare the results correctly.

I also stripped out everything related to mysql and mssql from those files. This is the series of regex I used to clean up the files:

find ./ -type f -name "*.test" -exec rename 's/\.test/\.slt/' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'hash-threshold 8\n' 'hash-threshold 8\ncontrol resultmode valuewise\n' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mysql.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'skipif mysql.+\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'skipif postgresql # PostgreSQL requires AS when renaming output columns\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'onlyif mssql.+\n.+\n.+\n\n' '' {} \;
find ./ -type f -name "*.slt" -exec sd -f i 'skipif mssql # not compatible\n' '' {} \;

After thinking about it a bit I think I could find a way to switch the results by using the column type count in query II to determine how many columns should exist and then do the rewrite. It wouldn't just be a regex (well, I'm sure someone could write some to deal with this but I'm not going there ... I've been burnt by regex far far too often in the past) but could be done with a small script.

Did anyone check the license of the sqlite test suite? Because if https://www.sqlite.org/sqllogictest/dir?ci=tip doesn't specify a license and https://www.sqlite.org/copyright.html doesn't explicitly mention the test corpus (which one could argue is a separate artifact). Hence you cannot just copy that data (i.e. it would be a copyright violation). Even reading it during tests over the internet might be a bit of a gray zone.

I'm going to push all of this to a df branch in my github account. I would really really like some assistance with evaluating the files and submitting tickets for any issues that were uncovered.

Yes, I will be 100% willing to help both review the errors and triage/file issues (and organize the effort)

I wll look into the copyright issue

https://github.com/hydromatic/sql-logic-test

Yes, though that wasn't where I grabbed them from. I think I grabbed the files from https://github.com/hydromatic/sql-logic-test

I didn't see the actual .test files, so I am not 100% sure what files we are referring to.

Official Git mirror of the SQLite source tree

https://github.com/sqlite/sqlite

https://github.com/sqlite/sqlite/blob/master/LICENSE.md explicitly says that this is in the public domain:

All of the test cases and testing code in the test/ directory

So TLDR I didn't find any compelling evidence either way related to licensing. When we have a proposal I will take a closer look

Even reading it during tests over the internet might be a bit of a gray zone.

At least according to my understanding of US copyright law, reading public files from the internet is not a problem. Redistributing them is where there can become a problem, depending on the license.

https://github.com/hydromatic/sql-logic-test

Yes, though that wasn't where I grabbed them from. I think I grabbed the files from https://github.com/hydromatic/sql-logic-test

I didn't see the actual .test files, so I am not 100% sure what files we are referring to.

https://github.com/hydromatic/sql-logic-test/tree/main/src/main/resources/test

As far as I could tell the files there are just copies of the ones from sqlite however I didn't have to deal with some oddball version control system to download them.

All of the test cases and testing code in the test/ directory

The sqllogictest files are not in that test directory. As best as I can tell they have no license attached to them at all if the license from sqlite doesn't cover them. The C source files do have licenses though.

Did anyone check the license of the sqlite test suite? Because if https://www.sqlite.org/sqllogictest/dir?ci=tip doesn't specify a license and https://www.sqlite.org/copyright.html doesn't explicitly mention the test corpus (which one could argue is a separate artifact). Hence you cannot just copy that data (i.e. it would be a copyright violation). Even reading it during tests over the internet might be a bit of a gray zone.

I asked the sqlite guys and they added a copyright.md file to the sqllogictest repo: https://sqlite.org/sqllogictest/file?name=COPYRIGHT.md&ci=tip

It's about as open as it's possible to make it :)

After thinking about it a bit I think I could find a way to switch the results by using the column type count in query II to determine how many columns should exist and then do the rewrite. It wouldn't just be a regex (well, I'm sure someone could write some to deal with this but I'm not going there ... I've been burnt by regex far far too often in the past) but could be done with a small script.

Oops, not so quick. I forgot about valuesort which is one of the result sorting methods. It isn't implemented in sqlogictest-rs (yet) and just switching that to rowsort doesn't work (causes errors)

I'll see about pushing a PR to that crate later today and then hopefully if merged we can just use the next version.

I've updated my test branch with the latest changes including adding skipif for Postgresql queries that fail to run. What is left is at least 19 legitimate query result mismatches where tickets were filed.

I've submitted a PR to the sqllogictest-rs repo for my changes but so far they haven't responded. If they do not get around to merging it for whatever reason we have two choices:

Rewrite the .slt files to be columnwise vs valuewise and change ordering to only be rowsort and remove the valuesort. I've actually written a lua script to handle the valuewise -> columnwise conversion, the second part will require some more work.
Fork the repo and use that.

For now I'm going to slow down work on this.

@Omega359

Thank you so much @Omega359

I created an epic to track the remaining work to complete the sqllogictest migration. I am hoping we can rally the community to help us finish it

[EPIC] Run full sqllogic / sqlite test suite against DataFusion #13811

I also field a ticket to track completing the integratio work:

Complete / integrate sqlite sqllogictest test scripts integrattion #13812

I am closing this issue for now so we can focus on completing #13811 -- please let me know if there are other items that should be tracked and I will file tickets

Thanks again

Navigation Menu

Repository navigation

[DISCUSSION] More SqlLogicTest test coverage for queries, including join queries #13470

9 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[DISCUSSION] More SqlLogicTest test coverage for queries, including join queries #13470

Description

Activity

alamb commented on Nov 19, 2024

Dandandan commented on Nov 22, 2024

Omega359 commented on Nov 24, 2024

Omega359 commented on Nov 29, 2024

schulte-lukas commented on Dec 1, 2024

Omega359 commented on Dec 1, 2024

alamb commented on Dec 2, 2024

comphead commented on Dec 2, 2024

alamb commented on Dec 2, 2024

comphead commented on Dec 2, 2024

Omega359 commented on Dec 2, 2024

alamb commented on Dec 2, 2024

Omega359 commented on Dec 3, 2024

gliga commented on Dec 4, 2024

gliga commented on Dec 4, 2024

9 remaining items

Omega359 commented on Dec 12, 2024

Omega359 commented on Dec 12, 2024

crepererum commented on Dec 12, 2024

alamb commented on Dec 12, 2024

alamb commented on Dec 12, 2024

alamb commented on Dec 12, 2024

alamb commented on Dec 12, 2024

Omega359 commented on Dec 12, 2024

Omega359 commented on Dec 12, 2024

Omega359 commented on Dec 12, 2024

Omega359 commented on Dec 12, 2024

Omega359 commented on Dec 14, 2024

alamb commented on Dec 17, 2024

alamb commented on Dec 17, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions