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

NaN and +/-Infinity needs to be quoted for floating point values in postgresql #444

Merged
merged 2 commits into from Feb 21, 2012

Conversation

kf8a
Copy link
Contributor

@kf8a kf8a commented Feb 20, 2012

It looks like postgres requires Nan and Infinity to be quoted when trying to insert them into a table. This is a tiny patch check for these values by overriding literal_float in the postgres adapter and adding the required quotes.

@jeremyevans
Copy link
Owner

I agree this should be fixed. However, your current patch will hurt performance, since it calls to_s on all Floats, and then does three string comparisons. For better performance, I would recommend something like:

if value.finite?
  super
elsif value > 0
  "'Infinity'"
elsif value < 0
  "'-Infinity'"
else
  "'NaN'"
end

Your specs should also check for correct results by actually inserting results in a table and reading the results back, not just checking that the expected SQL is produced.

If you can make those changes, I would appreciate it. If not, I'll try to take care of it myself before 3.33.0 is released.

…d to modified to account for nan and infinity
@kf8a
Copy link
Contributor Author

kf8a commented Feb 21, 2012

No problem, thanks for a great library.

I've modified the if statement as you suggested to reduce the work and added tests for the actual insertion and retrieval. I added conversions on the way out of the database in def float. I don't know if it's possible to reduce the comparisons on the way from the database. It might be possible to do something like:

def float(s)
    if s.to_f.to_s == s
       s.to_f
    else
      ...check for NaN and Infinity
    end
end

but that would be two type conversion plus one comparison vs 3 comparisons.

@jeremyevans
Copy link
Owner

I'll do some microbenchmarking to determine the fastest way. Otherwise, this looks good. I should be able to merge this later today. Thanks for the help!

@jeremyevans jeremyevans merged commit c66d288 into jeremyevans:master Feb 21, 2012
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.

None yet

2 participants