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 tests from expr.rs to sqllogictests. Part1 #8773

Merged
merged 3 commits into from
Jan 8, 2024
Merged

Conversation

comphead
Copy link
Contributor

@comphead comphead commented Jan 6, 2024

Which issue does this PR close?

Closes partially #8201.

Rationale for this change

Move tests from expr.rs to sqllogictests. Moved most of tests, other tests need more per-test attenttion

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 6, 2024
-- out of bounds
([5,4,3,2,1])[0],
([5,4,3,2,1])[6],
-- ([5,4,3,2,1])[-1], -- TODO: wrong answer
Copy link
Contributor

Choose a reason for hiding this comment

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

seem like we can return the correct answer

DataFusion CLI v34.0.0
❯ select [1,2,3,4][-1];
+------------------------------------------------------------+
| make_array(Int64(1),Int64(2),Int64(3),Int64(4))[Int64(-1)] |
+------------------------------------------------------------+
| 4                                                          |
+------------------------------------------------------------+
1 row in set. Query took 0.019 seconds.

❯ select [1,2,3,4][-4];
+------------------------------------------------------------+
| make_array(Int64(1),Int64(2),Int64(3),Int64(4))[Int64(-4)] |
+------------------------------------------------------------+
| 1                                                          |
+------------------------------------------------------------+
1 row in set. Query took 0.001 seconds.

❯ select [1,2,3,4][-5] is null;
+--------------------------------------------------------------------+
| make_array(Int64(1),Int64(2),Int64(3),Int64(4))[Int64(-5)] IS NULL |
+--------------------------------------------------------------------+
| true                                                               |
+--------------------------------------------------------------------+
1 row in set. Query took 0.001 seconds.

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 have checked with PG

select (array[1,2,3,4])[-1]
null

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'll create a followup issue to be in sync with PG

Copy link
Contributor

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

Epic work @comphead -- and thank you for the review @haohuaijin

There was one test I couldn't find ported but otherwise things look great here

[true, false],
['str1', 'str2'],
[[1,2], [3,4]],
[]
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 for adding the test that was commented out in the rs test

datafusion/sqllogictest/test_files/expr.slt Show resolved Hide resolved
0 years 12 mons 1 days 1 hours 1 mins 1.000000000 secs

query I
SELECT ascii('')
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually prefer this style (one query per expression) rather than the style earlier in the PR of one query with all the expressions as I find the output easier to understand

query T
SELECT chr(CAST(128175 AS int))
----
💯
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Ok(())
}

/// Test string expressions test split into two batches
Copy link
Contributor

Choose a reason for hiding this comment

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

it is sweet that that this is no longer a problem with sqllogictest (though to be fair this code should probably have been using functions rather than macros)

datafusion/sqllogictest/test_files/expr.slt Show resolved Hide resolved
comphead and others added 2 commits January 8, 2024 13:59
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@comphead comphead merged commit 0e53c6d into apache:main Jan 8, 2024
22 checks passed
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.

None yet

3 participants