Skip to content

fix: make tan and atan2 compatible#3849

Open
kazuyukitanimura wants to merge 10 commits intoapache:mainfrom
kazuyukitanimura:disable-atan2
Open

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

Conversation

@kazuyukitanimura
Copy link
Copy Markdown
Contributor

@kazuyukitanimura kazuyukitanimura commented Mar 31, 2026

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?

@andygrove
Copy link
Copy Markdown
Member

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

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

override def getSupportLevel(expr: Atan2): SupportLevel =
Incompatible(
Some(
"atan2(-0.0, -0.0) produces incompatible result" +
Copy link
Copy Markdown
Contributor

@comphead comphead Apr 3, 2026

Choose a reason for hiding this comment

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

if it is only 1 input value which returns incorrect response, should we just hardcode the response?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can done separately. I haven't decided how we should handle this. Because this is more like Spark side issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually we need do more tests for atan2 to enable e.g.

 atan2(-0.0, -0.0) 
 atan2(-0.0, +0.0)
 atan2(+0.0, -0.0)
 atan2(+0.0, +0.0)
 atan2(-0.0, -1.0) 
 atan2(-1.0, -0.0)
 atan2(-0.0, +1.0)
 atan2(+1.0, -0.0)
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@comphead I ended up fixing atan2 as well. Please take a look

@kazuyukitanimura kazuyukitanimura changed the title fix: disable atan2 instead of tan fix: make tan and atan2 compatible Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tan(-0.0) produces incorrect result

3 participants