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

Getting back different values between Time#to_r and Time#to_i #3433

Closed
alexpjohnson opened this issue Oct 27, 2015 · 4 comments
Closed

Getting back different values between Time#to_r and Time#to_i #3433

alexpjohnson opened this issue Oct 27, 2015 · 4 comments

Comments

@alexpjohnson
Copy link

@alexpjohnson alexpjohnson commented Oct 27, 2015

Sample test case:

t1 = Time.now.end_of_day
#2015-10-27 23:59:59 -0400
integer_val = t1.utc.to_i
#1446004799
integer_val.class
#Fixnum
rational_val = t1.utc.to_r
#1446004800
rational_val.class
#Fixnum
Time.at(integer_val)
#2015-10-27 23:59:59 -0400
Time.at(rational_val)
#2015-10-28 00:00:00 -0400

In this example the rounding differences causes us to slip a day. We believe the problem is on this line: https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/RubyTime.java#L747. By calling to_f before to_r we are losing precision and advancing the time.

@headius
Copy link
Member

@headius headius commented Oct 29, 2015

Ahh yes, We probably should port over the logic to turn a time directly into a rational, or perhaps convert to a BigDecimal first rather than a Float?

@alexpjohnson
Copy link
Author

@alexpjohnson alexpjohnson commented Oct 30, 2015

We managed to get past it by overriding Time#end_of_day to ignore the nanosecond precision. I'm not a C expert, but it seems the MRI code is going directly to a rational so maybe that is the correct path.

@headius
Copy link
Member

@headius headius commented Nov 19, 2015

Interestingly enough, fixing our to_r to return a proper Rational does not appear to fix it. Our construction of Time from Rational seems to still have a rounding issue:

Your script, expanded, with my to_r fix:

time: 2015-11-19 23:59:59 -0600
to_i: 1447999199
class: Fixnum
nsec: 999999999
class: Fixnum
utc.to_r: 1447999199999999999/1000000000
class: Rational
time at i: 2015-11-19 23:59:59 -0600
time at r: 2015-11-20 00:00:00 -0600

Continuing to investigate.

@headius headius closed this in d4ff615 Nov 19, 2015
headius added a commit that referenced this issue Nov 19, 2015
@headius headius added this to the JRuby 9.0.5.0 milestone Nov 19, 2015
@alexpjohnson
Copy link
Author

@alexpjohnson alexpjohnson commented Nov 20, 2015

Thanks a lot @headius 👍

headius added a commit to ruby/spec that referenced this issue Nov 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.