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

ActiveRecord decimal rounding regression in Ruby 3 #42098

Closed
soberstadt opened this issue Apr 28, 2021 · 11 comments
Closed

ActiveRecord decimal rounding regression in Ruby 3 #42098

soberstadt opened this issue Apr 28, 2021 · 11 comments

Comments

@soberstadt
Copy link

I believe this is being caused by the BigDecimal.to_d method (used https://github.com/rails/rails/blob/v6.1.3.1/activemodel/lib/active_model/type/decimal.rb#L48) that seems to have a behavior change in Ruby 3.0.0 (ruby/bigdecimal#185).

I believe we could make a patch to work around this issue:

module ActiveModel
  module Type
    class Decimal < Value
      def convert_float_to_big_decimal(value)
        if precision
          BigDecimal(apply_scale(value), float_precision)
        else
          value.to_d(::Float::DIG)
        end
      end
    end
  end
end

Float::DIG is 15 in ruby 3.0.1-2.4.9:
image

Steps to reproduce

This test passes in Ruby 2.7.3 but fails on Ruby 3.0.0

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "activerecord", "6.1.3.1"
  gem "sqlite3-ruby"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: "test.sqlite3")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :purchases, force: true do |t|
    t.decimal :amount
  end
end

class Purchase < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_to_sql
    query = Purchase.where(amount: 8.05)
    expected_sql = 'SELECT "purchases".* FROM "purchases" WHERE "purchases"."amount" = 8.05'
    assert_equal query.to_sql, expected_sql
  end
end

Passing test with patch:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "activerecord", "6.1.3.1"
  gem "sqlite3-ruby"
end

require "active_record"
require "minitest/autorun"
require "logger"

# monkey patch
module ActiveModel
  module Type
    class Decimal < Value
      def convert_float_to_big_decimal(value)
        if precision
          BigDecimal(apply_scale(value), float_precision)
        else
          value.to_d(::Float::DIG)
        end
      end
    end
  end
end

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: "test.sqlite3")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :purchases, force: true do |t|
    t.decimal :amount
  end
end

class Purchase < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_to_sql
    query = Purchase.where(amount: 8.05)
    expected_sql = 'SELECT "purchases".* FROM "purchases" WHERE "purchases"."amount" = 8.05'
    assert_equal query.to_sql, expected_sql
  end
end

Expected behavior

I expected the column filter to query based on the value provided: 8.05

Actual behavior

Rails converts the decimal to a floating point number very close to, but not exactly the provided value: 8.050000000000001

System configuration

Rails version:
6.1.3.1

Ruby version:
2.7.3, 3.0.0 (and 3.0.1)

@mrkn
Copy link
Contributor

mrkn commented May 1, 2021

This is not Rails's issue, but due to bigdecimal's recent behavior change.
The change was introduced for fixing the bug reported at https://bugs.ruby-lang.org/issues/13331.

If you need to keep the old behavior of to_d, please use the old version of bigdecimal.

I guess the next version of bigdecimal (it will be released until Ruby 3.1) can produce 8.05 by to_d.

Thanks.

@pixeltrix
Copy link
Contributor

@mrkn thanks for the information 👍🏻

@soberstadt since this is an issue in the bigdecimal gem we don't want to monkey patch the method - I'd follow the suggestion of using the old version until a newer version is released. Thanks for your report.

@kamipo
Copy link
Member

kamipo commented May 2, 2021

@mrkn Do you have any plan to release bigdecimal 3.0.1 which has the Changes entry ruby/bigdecimal@af6502c?

I've also been affected from the behavior change in testing #42125.

The behavior change of Float#to_d in bigdecimal 3.0.0 ("9.050000000000001" == 9.05.to_d.to_s("F")) is actually regarded as a regression for most existing apps (at least for me), and it is already fixed in ruby/bigdecimal#180.

I'd not like to catch the case in the tests just to pass CI on Ruby 3.0.x.

https://github.com/rails/rails/pull/42125/files#diff-de2bceb438d9de05d102343b9dad3045142d244675c57e16db102753ea48a2ceR105-R115

    # BigDecimal 3.0.0 has a rounding problem https://github.com/ruby/bigdecimal/issues/185
    # caused by fixing https://github.com/ruby/bigdecimal/issues/70.
    # It will be fixed by https://github.com/ruby/bigdecimal/pull/180
    # in BigDecimal 3.0.1.
    if "9.050000000000001" == 9.05.to_d.to_s("F")
      assert_equal BigDecimal("65.59999999999999"), subject.virtual_decimal_number
      assert_not_predicate subject, :valid?
    else
      assert_equal BigDecimal("65.6"), subject.virtual_decimal_number
      assert_predicate subject, :valid?
    end

@mrkn
Copy link
Contributor

mrkn commented May 3, 2021

@kamipo Oh, I totally forgot that I've already fixed that issue in bigdecimal...
OK. I'll release the new version of bigdecimal soon.

@mrkn
Copy link
Contributor

mrkn commented May 3, 2021

@kamipo @soberstadt I released bigdecimal 3.0.1. Please try to use it.

@kamipo
Copy link
Member

kamipo commented May 3, 2021

Thank you for the new release.

When I was testing #42125, I've found weird behavior in bigdecimal 3.0.1 ruby/bigdecimal#192.

@mrkn
Copy link
Contributor

mrkn commented May 3, 2021

Ah, I partially remembered the situation of the master branch. The fixing the precision handling in conversion has not been completed. It may be the reason why 3.0.1 didn't released.

I'll yank bigdecimal 3.0.1, release 3.0.2 to revert the change of 3.0.1, and restart the development asap.

Sorry for the inconvenience.

Please reopen this issue.

@pixeltrix pixeltrix reopened this May 3, 2021
waiting-for-dev added a commit to nebulab/solidus that referenced this issue May 20, 2021
Besides a couple of [issues related to the new separation between
keyword and positional
arguments](https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/),
this bug rails/rails#42098 is currently
hitting us. As we don't need to do anything from our side, we mark our
failing example so that we'll notice when it gets fixed upstream.
waiting-for-dev added a commit to nebulab/solidus that referenced this issue May 20, 2021
Besides a couple of [issues related to the new separation between
keyword and positional
arguments](https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/),
this bug rails/rails#42098 is currently
hitting us. As we don't need to do anything from our side, we mark our
failing example so that we'll notice when it gets fixed upstream.
@soberstadt
Copy link
Author

@mrkn thanks for your hard contributing to both Rails and BigDecimal! Any update on the state of this issue? This is still a barrier for us to upgrade our rails app to ruby 3.

@byroot
Copy link
Member

byroot commented Jul 27, 2021

For people stuck because of this, here's the patch we've been using since months: https://gist.github.com/casperisfine/f3f7b30d0ce4b4f41bc19eae097143b1

This issue was reported several times upstream, and there's pretty much nothing Rails can do, so I believe we should close.

@byroot byroot closed this as completed Jul 27, 2021
kennyadsl pushed a commit to solidusio/solidus that referenced this issue Sep 10, 2021
Besides a couple of [issues related to the new separation between
keyword and positional
arguments](https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/),
this bug rails/rails#42098 is currently
hitting us. As we don't need to do anything from our side, we mark our
failing example so that we'll notice when it gets fixed upstream.
@HarlemSquirrel
Copy link

Pinning bigdecimal to 2.0.0 seems to have fixed the issue for me with Ruby 3.0.2

gem "bigdecimal", "2.0.0"

@FunnyHector
Copy link

Just to add, the solution is either pinning BigDecimal version to 2.0.0, or use the patch mentioned above. I have confirmed that this regression is fixed in Ruby 3.1-preview version, so personally I think a patch is a better solution. Once 3.1 is out, the patch can be removed.

rmparr pushed a commit to rmparr/solidus that referenced this issue Jun 1, 2022
Besides a couple of [issues related to the new separation between
keyword and positional
arguments](https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/),
this bug rails/rails#42098 is currently
hitting us. As we don't need to do anything from our side, we mark our
failing example so that we'll notice when it gets fixed upstream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants