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

Port math.rs to sqllogictest #6037

Merged
merged 3 commits into from
Apr 19, 2023
Merged

Port math.rs to sqllogictest #6037

merged 3 commits into from
Apr 19, 2023

Conversation

2010YOUY01
Copy link
Contributor

Which issue does this PR close?

Part of #4495.

Rationale for this change

Ported tests in math.rs into sqllogictest.
The engine for sqllogictest will round big decimals to 12 digits, so the output of test cases is also round to 12 digits.

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 Apr 17, 2023
@2010YOUY01 2010YOUY01 marked this pull request as draft April 17, 2023 20:11
@2010YOUY01 2010YOUY01 marked this pull request as ready for review April 17, 2023 20:37
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@@ -87,3 +87,24 @@ query RRRRRRRR
SELECT round(125.2345, -3), round(125.2345, -2), round(125.2345, -1), round(125.2345), round(125.2345, 0), round(125.2345, 1), round(125.2345, 2), round(125.2345, 3)
----
0 100 130 125 125 125.2 125.23 125.235

# Create table for atan2 test
statement ok
Copy link
Contributor

Choose a reason for hiding this comment

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

its good to drop table after the test completed, or as alternative you can prepare data in WITH clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback, I think even creating a table is not necessary. Updated

@@ -87,3 +87,9 @@ query RRRRRRRR
SELECT round(125.2345, -3), round(125.2345, -2), round(125.2345, -1), round(125.2345), round(125.2345, 0), round(125.2345, 1), round(125.2345, 2), round(125.2345, 3)
----
0 100 130 125 125 125.2 125.23 125.235

# atan2
query RRRR
Copy link
Contributor

Choose a reason for hiding this comment

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

please include also tests with nulls. Its likely gap from prev implementation
atan2(null, 1.0), atan2(2.0, 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.

Sure, expression tests for null cases can also be found here, returning NULL seems to be expected

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

cool, thanks @2010YOUY01

@Weijun-H
Copy link
Member

Weijun-H commented Apr 18, 2023

LGTM 👍

@waynexia waynexia merged commit 9798fbc into apache:main Apr 19, 2023
@alamb
Copy link
Contributor

alamb commented Apr 20, 2023

Thank you @2010YOUY01 and everyone who reviewed this!

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.

7 participants