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

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

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

Comments

Projects
None yet
3 participants
@kares
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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 27, 2017

Member

Reduced repro:

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

Something about how we're handling the fractional seconds.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 27, 2017

Member

Also affects master (9.2).

Member

headius commented Nov 27, 2017

Also affects master (9.2).

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 27, 2017

Member

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;
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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Nov 27, 2017

Member

@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

Member

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 headius closed this in 4be8d66 Nov 28, 2017

eregon added a commit to ruby/spec that referenced this issue Dec 1, 2017

@sebastianguarin

This comment has been minimized.

Show comment
Hide comment
@sebastianguarin

sebastianguarin Jan 18, 2018

I think this fix has raised a different issue
#4989

sebastianguarin commented Jan 18, 2018

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 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