search

ged / ruby-pg

A PostgreSQL client library for Ruby
Other
796 stars 180 forks source link

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

Closed kwatch closed 2 months ago

kwatch commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

kwatch commented 3 weeks ago

Why do you avoid describing the detail of inconsistencies'? Please explain aboutinconsistencies'. 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.