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

Make IRComparer consider nans to be less than non-nans. #6626

Merged
merged 4 commits into from
Feb 22, 2022
Merged

Conversation

alexreinking
Copy link
Member

@alexreinking alexreinking commented Feb 21, 2022

Our IRComparer's compare_scalar believed that all floating point values are totally ordered, which is not true. NaNs always compare false in ordering comparisons.

Fixes #6624

@alexreinking alexreinking changed the title Make IRComparer consider nans to be less than non-nans. Fixes #6624 Make IRComparer consider nans to be less than non-nans. Feb 21, 2022
@LebedevRI
Copy link
Contributor

Does this need any "is in strict fp mode" check?

@alexreinking
Copy link
Member Author

Does this need any "is in strict fp mode" check?

I don't believe so, no. I don't think we ever want to consider nan equal to anything but nan for the purposes of comparing IR nodes.

@alexreinking
Copy link
Member Author

Sorry, I just realized I forgot to actually use the updated target in the test... going to fix that now.

@alexreinking
Copy link
Member Author

Failing test looks like a performance flake to me... what are your thoughts, @abadams?

@alexreinking
Copy link
Member Author

Leaving this for someone else to merge or patch further.

@abadams abadams merged commit a4ed033 into main Feb 22, 2022
@alexreinking alexreinking deleted the bugfix/6624 branch February 22, 2022 05:37
@apartridge
Copy link

Thanks for fixing this issue! Would it be possible to make a 13.0.5 release with this fix?

@alexreinking
Copy link
Member Author

@apartridge - that would be possible, but my bandwidth is somewhat limited until mid-week next week.

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.

select statement that contains NaN value always returns NaN
5 participants