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.

Sign up for GitHub

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

Jump to bottom

Rails parsing time ends with usec 1 instead of 0 #4866

Closed
kares opened this issue Nov 26, 2017 · 5 comments
Closed

Rails parsing time ends with usec 1 instead of 0 #4866

kares opened this issue Nov 26, 2017 · 5 comments
Assignees
@kares
Labels
Rails
Milestone
JRuby 9.1.15.0

Comments

@kares
Copy link
Member

kares commented Nov 26, 2017

Environment

did not dig further down the rabbit hole ...
the reproducer does not need a DB setup.

JRuby 9.1.14.0
ActiveRecord 5.0.6

Expected Behavior

MRI 2.3.3

2.3.3 :001 > require 'active_record'
 => true 
2.3.3 :004 > ActiveRecord::Type::Time.new.cast('2007-01-01 12:30:00.999900').usec
 => 0

Actual Behavior

jruby-9.1.14.0 :003 > require 'active_record'
 => true 
jruby-9.1.14.0 :004 > 
jruby-9.1.14.0 :005 >   ActiveRecord::Type::Time.new.cast('2007-01-01 12:30:00.999900').usec
 => 1
@kares kares added the Rails label Nov 26, 2017
@kares kares added this to the JRuby 9.1.15.0 milestone Nov 26, 2017
@headius
Copy link
Member

headius commented Nov 27, 2017

Reduced repro:

Time.utc(2000, 1, 1, 12, 30, 0, 9999r/10000).usec

Something about how we're handling the fractional seconds.

@headius
Copy link
Member

headius commented Nov 27, 2017

Also affects master (9.2).

@headius
Copy link
Member

headius commented Nov 27, 2017

This patch appears to work, perhaps because it does more of the nanos/micros math in Rational form before converting to a double.

@kares You had a patch in this code to adjust how it rounds. What do you think?

diff --git a/core/src/main/java/org/jruby/RubyTime.java b/core/src/main/java/org/jruby/RubyTime.java
index 395e948aca..a986c77d68 100644
--- a/core/src/main/java/org/jruby/RubyTime.java
+++ b/core/src/main/java/org/jruby/RubyTime.java
@@ -1554,9 +1554,17 @@ public class RubyTime extends RubyObject {
             boolean fractionalUSecGiven = args[6] instanceof RubyFloat || args[6] instanceof RubyRational;
 
             if (fractionalUSecGiven) {
-                double micros = RubyNumeric.num2dbl(args[6]);
-                time.dt = dt.withMillis(dt.getMillis() + (long) (micros / 1000));
-                nanos = (long) Math.rint((micros * 1000) % 1000000);
+                if (args[6] instanceof RubyRational) {
+                    RubyRational usecRat = (RubyRational) args[6];
+                    RubyRational nsecRat = (RubyRational) usecRat.op_mul(context, runtime.newFixnum(1000));
+                    double tmpNanos = nsecRat.getDoubleValue(context);
+                    time.dt = dt.withMillis((long) (dt.getMillis() + (tmpNanos / 1000000)));
+                    nanos = (long) tmpNanos % 1000000;
+                } else {
+                    double micros = RubyNumeric.num2dbl(args[6]);
+                    time.dt = dt.withMillis(dt.getMillis() + (long) (micros / 1000));
+                    nanos = (long) Math.rint((micros * 1000) % 1000000);
+                }
             } else {
                 int usec = int_args[4] % 1000;
                 int msec = int_args[4] / 1000;

@kares
Copy link
Member Author

kares commented Nov 27, 2017

@headius aah, interesting - so its a matter of rounding, thought JRuby passes a default 1 somewhere ...
looks good to me if all tests pass as before 🚢 it

headius added a commit that referenced this issue Nov 27, 2017
@headius
Add spec for Time.{utc,gm} with small fractional usec.

Verified

This commit was signed with the committer’s verified signature.
headius Charles Oliver Nutter
GPG key ID: 8E26FF248BC7AEAD
Verified
Learn about vigilant mode
7733a41 
See #4866.
eregon pushed a commit to ruby/spec that referenced this issue Dec 1, 2017
@headius @eregon
Add spec for Time.{utc,gm} with small fractional usec.
4cda555 
See jruby/jruby#4866.
@sebastianguarin
Copy link

I think this fix has raised a different issue
#4989

kares added a commit to kares/jruby that referenced this issue Mar 27, 2018
@kares
[fix] cast nsec nanos to long to avoid "overflow" with double value
682626e 
fixes jruby#4989 ... follow-up on jruby#4866 (4be8d66)
@kares kares self-assigned this Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kares kares

Labels
Rails
Projects
None yet
Milestone
JRuby 9.1.15.0
Development

No branches or pull requests
3 participants
@headius @kares @sebastianguarin