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

Implement column number / column type verification for sqllogictest #4499

Closed
Tracked by #4460
alamb opened this issue Dec 4, 2022 · 4 comments · Fixed by #5253
Closed
Tracked by #4460

Implement column number / column type verification for sqllogictest #4499

alamb opened this issue Dec 4, 2022 · 4 comments · Fixed by #5253
Labels
sqllogictest SQL Logic Tests (.slt)

Comments

@alamb
Copy link
Contributor

alamb commented Dec 4, 2022

https://duckdb.org/dev/sqllogictest/result_verification

The standard way of verifying results of queries is using the query statement, followed by the letter I times the number of columns that are expected in the result.

query II
SELECT 42, 84 UNION ALL SELECT 10, 20;
----
42	84
10	20

At the moment, the datafusion sqllogictest framework entirely ignores the number of I after the query command

The goal of this ticket is to update the framework (and tests) to error if the number of Is doesn't match the number of columns

@alamb alamb mentioned this issue Dec 4, 2022
28 tasks
@xudong963 xudong963 added the sqllogictest SQL Logic Tests (.slt) label Dec 4, 2022
@xxchan
Copy link
Contributor

xxchan commented Dec 7, 2022

related issue: risinglightdb/sqllogictest-rs#36

@alamb alamb changed the title Implement column number verification for sqllogictest Implement column number / column type verification for sqllogictest Jan 8, 2023
@melgenek
Copy link
Contributor

melgenek commented Feb 7, 2023

Hey, guys!
I'd like to work on this issue, but I am not sure which way to go.

DuckDB suggests using 'I' just to verify the number of columns.
Sqllogic test defines 'T'(text), 'I' (integer), 'R' (floating number).
CocroachDB defines even more types T, F(floating), R (decimal), B (boolean), O (oid), _ (any).

I think having more than one type is useful. For example, in order to verify that null and null produces a NULL of type boolean, I needed to use pg_type and arrow_type to verify the result types.
https://github.com/apache/arrow-datafusion/blob/7d2d51b651d076d720e2879661346babcda9734a/datafusion/core/tests/sqllogictests/test_files/pg_compat/pg_compat_type_coercion.slt#L98-L115

However, having a boolean type would've allowed writing a more concise test which would run for both Datafusion and Postgres

query BB
select null and null, null or null;
----
NULL NULL

Do you have preferences regarding the set of types?


sqllogictest-rs skips columns validation now, but defines columns I (integer), T (text), R (floating point), ? (any).

In order to use sqllogictest-rs one would need to define a type and a validation strategy. This would likely mean aligning with the other users of sqllogictest-rs.

I thought it would be more flexible to have a pluggable type and a validation strategy, so I opened a pr in sqllogictest-rs risinglightdb/sqllogictest-rs#160. I would appreciate your feedback.

@alamb
Copy link
Contributor Author

alamb commented Feb 7, 2023

I think having more than one type is useful.

I agree

As long as the --complete mode is able to populate the appropriate types automatically I am all for adding more specific types. It would be unfortunate if in order to write sqllogictests in datafusion you had to know a mapping from type to character code that has more than three entries :)

@melgenek
Copy link
Contributor

Hi.

I opened a pr to define the sqllogictest output types and check them #5253.

The --complete doesn't work yet. There is a pr in sqllogictest-rs to make it work risinglightdb/sqllogictest-rs#164.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants