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

Fix to refer Float::NAN and Float::INFINITY #577

Closed
wants to merge 1 commit into from

Conversation

kwatch
Copy link

@kwatch kwatch commented Aug 7, 2024

Currently, ruby-pg returns evaluated value of 0.0/0.0 when a query search result contains NaN, instead of Float:: NAN. This behavior is strange and not intuitive, especially when user checks values in query by val.equal?(Float::NAN) (faster) instead of val.is_a?(Float) && val.nan? (slower).

This commit fixes it to return Float::NAN.
Similarly, fix to use Float::INFINITY instead of 1.0/0.0.

Currently, ruby-pg returns evaluated value of `0.0/0.0` when a
query search result contains NaN, instead of `Float:: NAN`.
This behavior is strange and not intuitive, especially when user
checks values in query by `val.equal?(Float::NAN)` (faster)
instead of `val.is_a?(Float) && val.nan?` (slower).

This commit fixes it to return `Float::NAN`.
Similarly, fix to use `Float::INFINITY` instead of `1.0/0.0`.
@larskanis
Copy link
Collaborator

NAN's aren't intuitive per definition. That's why Float::NAN != Float::NAN

I'm uncertain that this is a useful change for two reasons:

  1. The binary float decoder also returns a NAN and not the Float::NAN
  2. There is no negative Float::INFINITY, so that checking for -INF is not possible by the proposed way.

IMHO using val.equal?(Float::NAN) is relying on an implementation detail and so is no good recommendation.

Maybe val&.nan? is good enough (assuming that the column is float)?

@kwatch
Copy link
Author

kwatch commented Aug 13, 2024

As you say, this pull request may not be very useful for many people. However, if it does not give any negative values to the pg library, please consider accepting it.

I think that eval "0.0/0.0" or eval "1.0/0.0" is not the proper way to get NaN or Infinity, and referring to Float::NAN or Float::INFINITY is the proper way in Ruby.
(Ruby doesn't provide Float::NEGATIVE_INFINITY, so this pull request doesn't change eval "-1.0/0.0".)

In programming, eval should be used only in case when the problem can be solved only by eval, I think.

Again: this pull request may not be very useful for many people, but if it does not give any negative values to the pg library, please consider accepting it.

@larskanis
Copy link
Collaborator

This PR introduces inconsistencies. This is the negative value. I tried to write a proper test for this behaviour change and noticed the additional complexity to differentiate the cases.

There's nothing wrong to use rb_eval in a C extension to express things that are not complicated in C.

@kwatch
Copy link
Author

kwatch commented Aug 13, 2024

This PR introduces inconsistencies.

Please tell me the details. I'm trying this PR on my project and it seems fine until now.

CI on GitHub reports some errors but they seems to be related to Ractor, so this PR doesn't seem to be the cause of that errors.

@larskanis
Copy link
Collaborator

I do not accept this pull request. Sorry! My concerns are written in my first comment and you didn't show better reasons other than a very little performance benefit.

@larskanis larskanis closed this Aug 14, 2024
@kwatch
Copy link
Author

kwatch commented Oct 5, 2024

Why do you avoid describing the detail of inconsistencies'? Please explain about inconsistencies'.
If you can't, it means that you are lying, and closed this issue with unfair reason.

You have the right to close issues, but you also have responsibility for what you said.

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.

2 participants