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

Time/Datetime conversion inconsistency with MRI #4810

Closed
rdubya opened this issue Oct 10, 2017 · 21 comments
Closed

Time/Datetime conversion inconsistency with MRI #4810

rdubya opened this issue Oct 10, 2017 · 21 comments
Labels

Comments

@rdubya
Copy link

@rdubya rdubya commented Oct 10, 2017

Environment

Tested with JRuby 9.1.12.0 and JRuby 9.1.13.0 and compared to MRI 2.3.1

Expected Behavior

  • A Datetime value should be able to be converted to a Time and back to a Datetime and still retain the original value to match MRI's functionality

Actual Behavior

  • The value at the end does not match the value at the beginning

Example Cases

JRuby 9.1.13.0

jruby-9.1.13.0 :001 > datetime = DateTime.parse('-0001-12-31T23:58:59+00:00')
 => #<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
jruby-9.1.13.0 :002 > datetime.to_time
 => -0001-12-31 19:02:57 -0456
jruby-9.1.13.0 :003 > datetime.to_time.to_datetime
 => #<DateTime: 0000-01-02T19:02:57-04:56 ((1721059j,86339s,0n),-17762s,2299161j)>
jruby-9.1.13.0 :004 > time = Time.new(-1, 12, 31, 23, 59, 59, '+00:00')
 => -0001-12-31 23:59:59 UTC
jruby-9.1.13.0 :005 > time.to_datetime
 => #<DateTime: 0000-01-02T23:59:59+00:00 ((1721059j,86399s,0n),+0s,2299161j)>
jruby-9.1.13.0 :006 > time.to_datetime.to_time
 => 0000-01-02 19:03:57 -0456

MRI 2.3.1

2.3.1 :001 > require 'time'
 => true
2.3.1 :002 > datetime = DateTime.parse('-0001-12-31T23:58:59+00:00')
 => #<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
2.3.1 :003 > datetime.to_time
 => -0001-12-31 18:58:59 -0500
2.3.1 :004 > datetime.to_time.to_datetime
 => #<DateTime: -0001-12-31T18:58:59-05:00 ((1721057j,86339s,0n),-18000s,2299161j)>
2.3.1 :005 > time = Time.new(-1, 12, 31, 23, 59, 59, '+00:00')
 => -0001-12-31 23:59:59 +0000
2.3.1 :006 > time.to_datetime
 => #<DateTime: -0001-12-31T23:59:59+00:00 ((1721057j,86399s,0n),+0s,2299161j)>
2.3.1 :007 > time.to_datetime.to_time
 => -0001-12-31 18:59:59 -0500
@rdubya
Copy link
Author

@rdubya rdubya commented Oct 10, 2017

Actually, I'm going to edit the title because I just verified that it isn't specific to BC times:

JRuby:

jruby-9.1.13.0 :001 > datetime = DateTime.parse('0003-12-31T23:58:59+00:00')
 => #<DateTime: 0003-12-31T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
jruby-9.1.13.0 :002 > datetime.to_time
 => 0003-12-31 19:02:57 -0456
jruby-9.1.13.0 :003 > datetime.to_time.to_datetime
 => #<DateTime: 0004-01-02T19:02:57-04:56 ((1722520j,86339s,0n),-17762s,2299161j)>

MRI:

2.3.1 :001 > require 'time'
 => true
2.3.1 :002 > datetime = DateTime.parse('0003-12-31T23:58:59+00:00')
 => #<DateTime: 0003-12-31T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
2.3.1 :003 > datetime.to_time
 => 0003-12-31 18:58:59 -0500
2.3.1 :004 > datetime.to_time.to_datetime
 => #<DateTime: 0003-12-31T18:58:59-05:00 ((1722518j,86339s,0n),-18000s,2299161j)>
@rdubya rdubya changed the title Time/Datetime conversion inconsistency for BC date Time/Datetime conversion inconsistency with MRI Oct 10, 2017
@rdubya
Copy link
Author

@rdubya rdubya commented Oct 10, 2017

Sorry, I left out the Java version information:

java version "1.8.0_77"
Java(TM) SE Runtime Environment (build 1.8.0_77-b03)
Java HotSpot(TM) 64-Bit Server VM (build 25.77-b03, mixed mode)
@rdubya
Copy link
Author

@rdubya rdubya commented Oct 11, 2017

I just tracked down that the point where this quirkiness starts to happen is in the years 1883-1884. Newer years appear to work correctly, older years are converted incorrectly.

@headius
Copy link
Member

@headius headius commented Oct 30, 2017

This is likely a similar problem to #4822, where it appears we use different logic to resolve historical dates. I can reproduce locally, and changing the chronology we use seems to help but does not completely solve it.

Stock 9.1.14:

#<DateTime: 0003-12-31T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
0003-12-31 18:08:23 -0550
#<DateTime: 0004-01-02T18:08:23-05:50 ((1722520j,86339s,0n),-21036s,2299161j)>

9.1.14 using ISO chronology in date.rb instead of Gregorian/Julian:

#<DateTime: 0003-12-29T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
0003-12-29 18:08:23 -0550
#<DateTime: 0003-12-29T18:08:23-05:50 ((1722518j,86339s,0n),-21036s,2299161j)>
@headius
Copy link
Member

@headius headius commented Oct 30, 2017

Note that the times in my last result are the same moment but have acquired a different timezone/offset.

@headius
Copy link
Member

@headius headius commented Oct 30, 2017

Oh I just realized that the last result from @rdubya also localizes the time zone, so it seems like the only fix needed is to use ISO chronology. The time zone localization was fixed in 2.4.

@eregon My patch is obviously just brute force. I assume you arrived at this collection of conditionals for a reason...can you offer some insight into how we should handle this and #4822?

@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

I think the fix should be in Time#to_datetime, which seems buggy.
One simple way is to use DateTime.civil(year, month, day, h, min, s, offset), that should trivially fix it.
It seems Time#to_datetime creates a DateTime with the default calendar reform (ITALY), so nothing special to do there.

@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

The #chronology helper method should be used for Date itself, but not for Time I think, since the chronology of a time is always fixed to the default GJ calendar with the italy reform.

@headius
Copy link
Member

@headius headius commented Oct 30, 2017

@eregon I'm a bit confused. You said:

I think the fix should be in Time#to_datetime, which seems buggy.

And then you said:

It seems Time#to_datetime creates a DateTime with the default calendar reform (ITALY), so nothing special to do there.

Is it buggy or is there nothing to do?

@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

I meant there should be no need to mess with a chronology here, a Time converted to a DateTime just uses the default chronology.
Changing Time#to_datetime to use DateTime.civil(year, month, day, h, min, s, offset) should fix the problem.

@headius
Copy link
Member

@headius headius commented Oct 30, 2017

Ok, @eregon's suggested change does appear to fix #4822:

[--dev] ~/projects/jruby $ jruby time_bug_4822.rb 
1589-09-26 02:40:00 UTC
#<DateTime: 1589-09-26T02:40:00+00:00 ((2301699j,9600s,0n),+0s,2299161j)>
1573-11-22 01:46:40 UTC
#<DateTime: 1573-11-22T01:46:40+00:00 ((2295922j,6400s,0n),+0s,2299161j)>

[--dev] ~/projects/jruby $ /usr/bin/ruby time_bug_4822.rb 
1589-09-26 02:40:00 UTC
#<DateTime: 1589-09-26T02:40:00+00:00 ((2301699j,9600s,0n),+0s,2299161j)>
1573-11-22 01:46:40 UTC
#<DateTime: 1573-11-22T01:46:40+00:00 ((2295922j,6400s,0n),+0s,2299161j)>

But it still does not match MRI for this bug:

[--dev] ~/projects/jruby $ jruby -rdate -e "datetime = DateTime.parse('0003-12-31T23:58:59+00:00'); p datetime; p datetime.to_time; p datetime.to_time.to_datetime"
#<DateTime: 0003-12-31T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
0003-12-31 18:08:23 -0550
#<DateTime: 0003-12-31T18:08:23-06:00 ((1722518j,86903s,0n),-21600s,2299161j)>

[--dev] ~/projects/jruby $ ~/.rubies/ruby-2.3.0/bin/ruby -rdate -e "datetime = DateTime.parse('0003-12-31T23:58:59+00:00'); p datetime; p datetime.to_time; p datetime.to_time.to_datetime"
#<DateTime: 0003-12-31T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
0003-12-31 17:58:59 -0600
#<DateTime: 0003-12-31T17:58:59-06:00 ((1722518j,86339s,0n),-21600s,2299161j)>
@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

@headius You should compare with MRI 2.4.0, before that MRI doesn't respect the timezone on DateTime#to_time.

@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

Ah but this issue is for JRuby 9.1, nevermind then.

@headius
Copy link
Member

@headius headius commented Oct 30, 2017

@eregon But I'm fixing this for 9.1, which is compatible with 2.3.0. And the time doesn't match anyway.

@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

Trying it locally I get:

$ ruby -v -rdate -e "datetime = DateTime.parse('0003-12-31T23:58:59+00:00'); p datetime; p datetime.to_time; p datetime.to_time.to_datetime"
jruby 9.1.13.0 (2.3.3) 2017-09-06 8e1c115 OpenJDK 64-Bit Server VM 25.131-b12 on 1.8.0_131-b12 +jit [linux-x86_64]

#<DateTime: 0003-12-31T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
0004-01-01 01:04:20 +0105
#<DateTime: 0004-01-03T01:04:20+01:05 ((1722521j,-61s,0n),+3921s,2299161j)>
ruby -v -rdate -e   3.44s user 0.12s system 260% cpu 1.367 total

$ chruby 2.3.4          
$ ruby -v -rdate -e "datetime = DateTime.parse('0003-12-31T23:58:59+00:00'); p datetime; p datetime.to_time; p datetime.to_time.to_datetime"
ruby 2.3.4p301 (2017-03-30 revision 58214) [x86_64-linux]

#<DateTime: 0003-12-31T23:58:59+00:00 ((1722518j,86339s,0n),+0s,2299161j)>
0004-01-01 01:04:20 +0105
#<DateTime: 0004-01-01T01:04:20+01:05 ((1722518j,86339s,0n),+3921s,2299161j)>

So the Times do match at me.
But the Date do not, JRuby returns January 3rd.
There also seems to be something different for the zone offset.

@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

(My example above is without patch)

@eregon
Copy link
Member

@eregon eregon commented Oct 30, 2017

Could MRI be rounding the timezone from -0550 to -0600 at you?
What's your TZ zone? ($ readlink /etc/localtime)?

@headius
Copy link
Member

@headius headius commented Oct 31, 2017

@eregon I'm in America/Chicago TZ.

Still unclear what the right fixes are. It would be nice to iron this out for 9.1.14.

@eregon
Copy link
Member

@eregon eregon commented Nov 3, 2017

This should be fixed by 8da4e66 and spec added in 6c7ccbe.
Also related, the same for Time#to_date: 36d6d58.
@headius I let you backport those to 9.1.

Both of these fixes use the original definition from date.rb in 1.9.2:
https://github.com/ruby/ruby/blob/ruby_1_9_2/lib/date.rb

Those methods were changed to optimize with Joda-DateTime but obviously the corner cases are wrong (e.g.: Joda throws an error when given a date which is in the middle of the reform) and there seems to be no simple solution, so I reverted to the original code.

@rdubya
Copy link
Author

@rdubya rdubya commented Nov 7, 2017

It looks like DateTime#to_time is still ending up with a weird timezone issue for the example above. It does look like these changes fixed #4822 though

@headius
Copy link
Member

@headius headius commented Nov 8, 2017

@rdubya JRuby appears to match expectations now. I think you may be seeing the difference between Ruby 2.4 (and later 2.3) that does not localize during to_time and 2.3 and earlier that still does.

Here's my results for JRuby 9.1 versus Ruby 2.3:

$ TZ=EST jruby -v time_bug_4810.rb 
jruby 9.1.14.0-SNAPSHOT (2.3.3) 2017-11-08 7e7a8f3 Java HotSpot(TM) 64-Bit Server VM 25.131-b11 on 1.8.0_131-b11 +jit [darwin-x86_64]
#<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
-0001-12-31 18:58:59 -0500
#<DateTime: -0001-12-31T18:58:59-05:00 ((1721057j,86339s,0n),-18000s,2299161j)>
-0001-12-31 23:59:59 UTC
#<DateTime: -0001-12-31T23:59:59+00:00 ((1721057j,86399s,0n),+0s,2299161j)>
-0001-12-31 18:59:59 -0500

$ (chruby 2.3.4 ; TZ=EST ruby -v time_bug_4810.rb)
ruby 2.3.4p301 (2017-03-30 revision 58214) [x86_64-darwin16]
#<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
-0001-12-31 18:58:59 -0500
#<DateTime: -0001-12-31T18:58:59-05:00 ((1721057j,86339s,0n),-18000s,2299161j)>
-0001-12-31 23:59:59 +0000
#<DateTime: -0001-12-31T23:59:59+00:00 ((1721057j,86399s,0n),+0s,2299161j)>
-0001-12-31 18:59:59 -0500

And JRuby master (9.2) versus Ruby 2.4:

$ TZ=EST jruby -v time_bug_4810.rb 
jruby 9.2.0.0-SNAPSHOT (2.4.1) 2017-11-08 2e10a4e Java HotSpot(TM) 64-Bit Server VM 25.131-b11 on 1.8.0_131-b11 +jit [darwin-x86_64]
#<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
-0001-12-31 23:58:59 UTC
#<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
-0001-12-31 23:59:59 UTC
#<DateTime: -0001-12-31T23:59:59+00:00 ((1721057j,86399s,0n),+0s,2299161j)>
-0001-12-31 23:59:59 UTC

$ (chruby 2.4.1 ; TZ=EST ruby -v time_bug_4810.rb)
ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-darwin16]
#<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
-0001-12-31 23:58:59 +0000
#<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
-0001-12-31 23:59:59 +0000
#<DateTime: -0001-12-31T23:59:59+00:00 ((1721057j,86399s,0n),+0s,2299161j)>
-0001-12-31 23:59:59 +0000

As you can see, the results match.

There is an oddity on my local system where the time zone is coming back as -0550 rather than -0600 (I am in US Central Time, America/Chicago) but I don't think it's related to this issue.

 $ jruby -v time_bug_4810.rb 
jruby 9.1.14.0-SNAPSHOT (2.3.3) 2017-11-08 7e7a8f3 Java HotSpot(TM) 64-Bit Server VM 25.131-b11 on 1.8.0_131-b11 +jit [darwin-x86_64]
#<DateTime: -0001-12-31T23:58:59+00:00 ((1721057j,86339s,0n),+0s,2299161j)>
-0001-12-31 18:08:23 -0550
#<DateTime: -0001-12-31T18:08:23-05:50 ((1721057j,86339s,0n),-21036s,2299161j)>
-0001-12-31 23:59:59 UTC
#<DateTime: -0001-12-31T23:59:59+00:00 ((1721057j,86399s,0n),+0s,2299161j)>
-0001-12-31 18:09:23 -0550
@headius headius closed this Nov 8, 2017
@headius headius added this to the JRuby 9.1.14.0 milestone Nov 8, 2017
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
3 participants
You can’t perform that action at this time.