Skip to content

fix: make tan and atan2 compatible#3849

Merged
kazuyukitanimura merged 10 commits intoapache:mainfrom
kazuyukitanimura:disable-atan2
Apr 10, 2026
Merged

fix: make tan and atan2 compatible#3849
kazuyukitanimura merged 10 commits intoapache:mainfrom
kazuyukitanimura:disable-atan2

Conversation

@kazuyukitanimura
Copy link
Copy Markdown
Contributor

@kazuyukitanimura kazuyukitanimura commented Mar 31, 2026 •
edited
Loading

Which issue does this PR close?

Closes: #1897

Rationale for this change

#1897 claims tan is incompatible; however, what is really incompatible is atan2 that is failing in the same test in #1896

The correct results are

 atan2(-0.0, -0.0) =  -π    <= Comet answer
 atan2(-0.0, +0.0) = -0.0
 atan2(+0.0, -0.0) = +π
 atan2(+0.0, +0.0) = +0.0   <= Spark answer

Looks like Spark adds +0.0 to inputs in order to convert atan2(-0.0, -0.0) to atan2(+0.0, +0.0)

What changes are included in this PR?

Fixed atan2 and enabled tan

How are these changes tested?

@kazuyukitanimura kazuyukitanimura mentioned this pull request Mar 31, 2026
Closed
@andygrove
Copy link
Copy Markdown
Member

andygrove commented Mar 31, 2026

Thanks @kazuyukitanimura. Could you add SQL file based tests, since this is preferred approach now. See #3854 for example.

@kazuyukitanimura
Copy link
Copy Markdown
Contributor Author

kazuyukitanimura commented Mar 31, 2026

Thanks @kazuyukitanimura. Could you add SQL file based tests, since this is preferred approach now. See #3854 for example.

Thanks @andygrove, updated spark/src/test/resources/sql-tests/expressions/math/atan2.sql

andygrove
andygrove reviewed Apr 2, 2026
View reviewed changes
@kazuyukitanimura kazuyukitanimura mentioned this pull request Apr 3, 2026
Open
comphead
comphead reviewed Apr 3, 2026
View reviewed changes
@kazuyukitanimura kazuyukitanimura changed the title fix: disable atan2 instead of tan fix: make tan and atan2 compatible Apr 3, 2026
comphead
comphead reviewed Apr 10, 2026
View reviewed changes
spark/src/test/resources/sql-tests/expressions/math/atan2.sql
statement
INSERT INTO test_atan2 VALUES (0.0, 1.0), (1.0, 0.0), (1.0, 1.0), (-1.0, -1.0), (0.0, 0.0), (NULL, 1.0), (1.0, NULL), (cast('NaN' as double), 1.0), (cast('Infinity' as double), 1.0)
INSERT INTO test_atan2 VALUES
(0.0, 0.0), (0.0, -0.0), (0.0, 1.0), (0.0, -1.0), (0.0, NULL), (0.0, cast('NaN' as double)), (0.0, cast('Infinity' as double)), (0.0, cast('-Infinity' as double)),
Copy link
Copy Markdown
Contributor

@comphead comphead Apr 10, 2026

Choose a reason for hiding this comment

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

nice permutations

comphead
comphead approved these changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown
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.

Thanks @kazuyukitanimura this is LGTM

@kazuyukitanimura kazuyukitanimura merged commit 88c1ffc into apache:main Apr 10, 2026
159 checks passed
@kazuyukitanimura
Copy link
Copy Markdown
Contributor Author

kazuyukitanimura commented Apr 10, 2026

Thank you @comphead merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andygrove andygrove andygrove left review comments

@comphead comphead comphead approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tan(-0.0) produces incorrect result

3 participants

@kazuyukitanimura @andygrove @comphead