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.

Sign up for GitHub

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

Jump to bottom

Make sqllogictest distinguish NaN from -NaN #7403

Merged
merged 3 commits into from
Aug 25, 2023
Merged

Make sqllogictest distinguish NaN from -NaN #7403

merged 3 commits into from
Aug 25, 2023

Conversation

sarutak
Copy link
Member

@sarutak sarutak commented Aug 25, 2023 •
edited by viirya
Loading

Which issue does this PR close?

Closes #7402

Rationale for this change

DataFusion distinguishes NaN and -NaN so sqllogictest should distinguish them for precise test.

What changes are included in this PR?

Changed conversion.rs so that it returns "-NaN" if a value is a kind of NaN and its sign is negative.

Are these changes tested?

Modified existing test and added a simple test to select.rs.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Aug 25, 2023
@viirya
Copy link
Member

viirya commented Aug 25, 2023

pyarrow error looks unrelated. Maybe re-trigger it later.

sarutak
sarutak commented Aug 25, 2023
View reviewed changes
datafusion/sqllogictest/test_files/math.slt
query RRR
SELECT nanvl(asin(10), 1.0), nanvl(1.0, 2.0), nanvl(asin(10), asin(10))
query RRB
SELECT nanvl(asin(10), 1.0), nanvl(1.0, 2.0), isnan(nanvl(asin(10), asin(10)))
Copy link
Member Author

@sarutak sarutak Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The sign of NaN returned by asin(10) seems different between operating systems.
On Linux, NaN is returned but -NaN on macOS and WIndows.
https://github.com/apache/arrow-datafusion/actions/runs/5971983397/job/16201846083#step:5:5969
https://github.com/apache/arrow-datafusion/actions/runs/5971983397/job/16201846201#step:5:6058

So, I use isnan just to check if it's NaN or not.

@alamb
Copy link
Contributor

alamb commented Aug 25, 2023

I believe CI will pass after merging up from main to get the change in #7416

@alamb
Copy link
Contributor

alamb commented Aug 25, 2023

I merged up from main to get the CI test fix

@alamb alamb merged commit fd7e612 into apache:main Aug 25, 2023
21 checks passed
@jonahgao jonahgao mentioned this pull request Sep 1, 2023
Closed
@berkaysynnada
Copy link
Contributor

berkaysynnada commented Sep 1, 2023 •
edited
Loading

@alamb We have encountered a problem related to this PR. The sign bit of the value representing NaN is platform-dependent. (As seen in this issue, I think the result of scalar functions on ARM is always NaN, not -NaN.). Can you suggest a way to solve this issue in a deterministic way? isnan() cannot solve the problem for columnar data.

@sarutak
Copy link
Member Author

sarutak commented Sep 1, 2023 •
edited
Loading

Hi @berkaysynnada
Can we modify the query in sqllogictests using nanvl?
Or, just revert this change.

@alamb @viirya What do you think?

sarutak added a commit to sarutak/arrow-datafusion that referenced this pull request Sep 1, 2023
@sarutak
Revert "Make sqllogictest distinguish NaN from -NaN (apache#7403)"
e74cb3c 
This reverts commit fd7e612.
@sarutak sarutak mentioned this pull request Sep 1, 2023
Merged
@sarutak
Copy link
Member Author

sarutak commented Sep 1, 2023 •
edited
Loading

@berkaysynnada I opened #7462 for the issue you hit.
Does sqllogictest work on your machine with that change?

alamb pushed a commit that referenced this pull request Sep 7, 2023
@sarutak
Make sqllogictest platform-independent for the sign of NaN (#7462)
7c6d731 
* Revert "Make sqllogictest distinguish NaN from -NaN (#7403)"

This reverts commit fd7e612.

* Make sqllogictests platform-independent for the sign of NaNs

* Add comment

* Ensure -NaN is parsed as -NaN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viirya viirya viirya approved these changes

Assignees
No one assigned
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sqllogictest should distinguish NaN from -NaN
4 participants
@sarutak @viirya @alamb @berkaysynnada