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

Time.at loses precision when using BigDecimal as argument #3616

Closed
alex88 opened this Issue Jan 26, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@alex88

alex88 commented Jan 26, 2016

I was debugging why logstash was losing precision with a gelf input, it ended up to be a jRuby bug (maybe).

These are the steps to reproduce the issue:

➜  ~  rbenv shell jruby-9.0.4.0
➜  ~  ruby -v
jruby 9.0.4.0 (2.2.2) 2015-11-12 b9fb7aa Java HotSpot(TM) 64-Bit Server VM 25.25-b02 on 1.8.0_25-b17 +jit [darwin-x86_64]
➜  ~  irb
irb(main):001:0> RUBY_ENGINE
=> "jruby"
irb(main):002:0> RUBY_VERSION
=> "2.2.2"
irb(main):003:0> require 'time'
=> true
irb(main):004:0> require 'bigdecimal'
=> true
irb(main):005:0> bd = BigDecimal.new('0.1453769667458E10')
=> #<BigDecimal:76a3e297,'0.1453769667458E10',13(16)>
irb(main):006:0> bd.to_f
=> 1453769667.458
irb(main):007:0> Time.at(bd).to_f
=> 1453769667.0
irb(main):008:0> Time.at(bd.to_f).to_f
=> 1453769667.458
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 26, 2016

Member

This is a known issue...as in I've known about it. MRI represents their Time object internally with much more type flexibility...Bignums, Bigdecimals, Rationals. We do not have this flexibility wired into our various Time constructors and methods.

In this case, I'd guess we're simply truncating to Fixnum any input to Time.at.

@alex88 I wonder if you'd like to expand the test cases for Time.at (and friends) in https://github.com/ruby/spec? It appears we're failing two cases related to to_r coercion, but it looks like the available cases could be improved.

Member

headius commented Jan 26, 2016

This is a known issue...as in I've known about it. MRI represents their Time object internally with much more type flexibility...Bignums, Bigdecimals, Rationals. We do not have this flexibility wired into our various Time constructors and methods.

In this case, I'd guess we're simply truncating to Fixnum any input to Time.at.

@alex88 I wonder if you'd like to expand the test cases for Time.at (and friends) in https://github.com/ruby/spec? It appears we're failing two cases related to to_r coercion, but it looks like the available cases could be improved.

@alex88

This comment has been minimized.

Show comment
Hide comment
@alex88

alex88 Jan 26, 2016

@headius correct me if I'm wrong, but you're not truncating to Fixnum, as Time.at(bd.to_f).to_f works fine.

Anyway, I'm gonna work on ruby/spec to add the tests with BigNumber later this week

alex88 commented Jan 26, 2016

@headius correct me if I'm wrong, but you're not truncating to Fixnum, as Time.at(bd.to_f).to_f works fine.

Anyway, I'm gonna work on ruby/spec to add the tests with BigNumber later this week

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 26, 2016

Member

I have a fix for this based on a quick look at MRI's code. The two main issues were that we forced non-Float/Rational to Fixnum (rather than coercing to Rational), and our BigDecimal.to_r was broken.

Member

headius commented Jan 26, 2016

I have a fix for this based on a quick look at MRI's code. The two main issues were that we forced non-Float/Rational to Fixnum (rather than coercing to Rational), and our BigDecimal.to_r was broken.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 26, 2016

Member

@alex88 The incoming value for Time.at is intended to represent fractional seconds. As shown in your example, when passing a BigDecimal, we end up with an integral number of seconds from Time#to_f because we truncate to Fixnum.

Member

headius commented Jan 26, 2016

@alex88 The incoming value for Time.at is intended to represent fractional seconds. As shown in your example, when passing a BigDecimal, we end up with an integral number of seconds from Time#to_f because we truncate to Fixnum.

@headius headius closed this in 16b13e1 Jan 26, 2016

@headius headius added this to the JRuby 9.1.0.0 milestone Jan 26, 2016

@alex88

This comment has been minimized.

Show comment
Hide comment
@alex88

alex88 Jan 26, 2016

Wow that was fast, thanks @headius!

alex88 commented Jan 26, 2016

Wow that was fast, thanks @headius!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2016

Member

@alex88 You're welcome! Could you add a couple quick specs to https://github.com/ruby/spec so we've got coverage?

Member

headius commented Feb 14, 2016

@alex88 You're welcome! Could you add a couple quick specs to https://github.com/ruby/spec so we've got coverage?

@alex88

This comment has been minimized.

Show comment
Hide comment
@alex88

alex88 Feb 15, 2016

@headius doing that now, however jruby 9.0.4.0 have like 143 failures already, is that expected?

PS: I'm running ruby specs inside an rbenv shell jruby-9.0.4.0 if that matters

alex88 commented Feb 15, 2016

@headius doing that now, however jruby 9.0.4.0 have like 143 failures already, is that expected?

PS: I'm running ruby specs inside an rbenv shell jruby-9.0.4.0 if that matters

@alex88

This comment has been minimized.

Show comment
Hide comment
@alex88

alex88 Feb 15, 2016

For example the time specs:

➜  ruby-spec git:(master) ../mspec/bin/mspec core/time/at_spec.rb
jruby 9.0.4.0 (2.2.2) 2015-11-12 b9fb7aa Java HotSpot(TM) 64-Bit Server VM 25.25-b02 on 1.8.0_25-b17 +jit [darwin-x86_64]

1)
Time.at passed Numeric roundtrips a Rational produced by #to_r FAILED
Expected 2016-02-14 16:27:27 -0800
 to equal 2016-02-14 16:27:27 -0800

/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:29:in `block in (root)'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

2)
Time.at passed non-Time, non-Numeric with an argument that responds to #to_r coerces using #to_r ERROR
NoMethodError: undefined method `to_i' for #<NumericMockObject:0x1563da5 @name="rational", @null=nil>
org/jruby/RubyBasicObject.java:1592:in `method_missing'
org/jruby/RubyNumeric.java:773:in `to_int'
org/jruby/RubyTime.java:1025:in `at'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:85:in `block in (root)'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

3)
An exception occurred during: Mock.verify_count
Time.at passed non-Time, non-Numeric with an argument that responds to #to_r coerces using #to_r FAILED
Mock 'rational' expected to receive 'to_r' exactly 1 times
but received it 0 times
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyHash.java:1343:in `each'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

4)
Time.at with a second argument that responds to #to_r coerces using #to_r ERROR
NoMethodError: undefined method `to_i' for #<NumericMockObject:0x480bdb19 @name="rational", @null=nil>
org/jruby/RubyBasicObject.java:1592:in `method_missing'
org/jruby/RubyNumeric.java:773:in `to_int'
org/jruby/RubyTime.java:1086:in `at'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:116:in `block in (root)'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

5)
An exception occurred during: Mock.verify_count
Time.at with a second argument that responds to #to_r coerces using #to_r FAILED
Mock 'rational' expected to receive 'to_r' exactly 1 times
but received it 0 times
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyHash.java:1343:in `each'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'
[- | ==================100%================== | 00:00:00]      3F      2E

alex88 commented Feb 15, 2016

For example the time specs:

➜  ruby-spec git:(master) ../mspec/bin/mspec core/time/at_spec.rb
jruby 9.0.4.0 (2.2.2) 2015-11-12 b9fb7aa Java HotSpot(TM) 64-Bit Server VM 25.25-b02 on 1.8.0_25-b17 +jit [darwin-x86_64]

1)
Time.at passed Numeric roundtrips a Rational produced by #to_r FAILED
Expected 2016-02-14 16:27:27 -0800
 to equal 2016-02-14 16:27:27 -0800

/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:29:in `block in (root)'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

2)
Time.at passed non-Time, non-Numeric with an argument that responds to #to_r coerces using #to_r ERROR
NoMethodError: undefined method `to_i' for #<NumericMockObject:0x1563da5 @name="rational", @null=nil>
org/jruby/RubyBasicObject.java:1592:in `method_missing'
org/jruby/RubyNumeric.java:773:in `to_int'
org/jruby/RubyTime.java:1025:in `at'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:85:in `block in (root)'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

3)
An exception occurred during: Mock.verify_count
Time.at passed non-Time, non-Numeric with an argument that responds to #to_r coerces using #to_r FAILED
Mock 'rational' expected to receive 'to_r' exactly 1 times
but received it 0 times
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyHash.java:1343:in `each'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

4)
Time.at with a second argument that responds to #to_r coerces using #to_r ERROR
NoMethodError: undefined method `to_i' for #<NumericMockObject:0x480bdb19 @name="rational", @null=nil>
org/jruby/RubyBasicObject.java:1592:in `method_missing'
org/jruby/RubyNumeric.java:773:in `to_int'
org/jruby/RubyTime.java:1086:in `at'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:116:in `block in (root)'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'

5)
An exception occurred during: Mock.verify_count
Time.at with a second argument that responds to #to_r coerces using #to_r FAILED
Mock 'rational' expected to receive 'to_r' exactly 1 times
but received it 0 times
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyHash.java:1343:in `each'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1552:in `all?'
org/jruby/RubyFixnum.java:301:in `times'
org/jruby/RubyArray.java:1560:in `each'
org/jruby/RubyArray.java:1560:in `each'
/Users/alessandro/Sites/ruby-spec/core/time/at_spec.rb:3:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1560:in `each'
[- | ==================100%================== | 00:00:00]      3F      2E
@alex88

This comment has been minimized.

Show comment
Hide comment
@alex88

alex88 Feb 15, 2016

Just tried setting also jruby as global default implementation, same thing

alex88 commented Feb 15, 2016

Just tried setting also jruby as global default implementation, same thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment