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

Make subtract and addition operators work with DateTime objects #5369

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@massive
Copy link
Contributor

massive commented Oct 15, 2018

We recently discovered a few nasty bugs that affect DateTime calculations when the precision of time is high enough.

As a practical example, when subtracting values produced by DateTime.current (via Active Support), you'll get weird results. e.g.:

[4] pry(main)> DateTime.current - DateTime.current
17841599/43200000
[5] pry(main)> DateTime.current - DateTime.current
-609/1000

I traced the problem down to RubyDate#op_plus_numeric, where value multiplication produce incorrect results seemingly due to floating point inaccuracy. I was able to "fix" the problem using round, but obviously that is not really elegant solution (maybe it should use BigDecimal under the hood or something?).

While I was working with this bug, I also discovered another bug, which turned out to be related: namely when adding sub-millisecond precision value to DateTime object, JRuby interpreter threw an error.

Both of the bugs are reproducible using provided specs.

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 15, 2018

nice - thanks, isn't using longs enough? that round with a hard-coded precision of 10 just looks weird.
best approach would be to detect long overflows on num/del and simply handle those in a different branch.

@kares kares added this to the JRuby 9.2.1.0 milestone Oct 15, 2018

@kares kares self-assigned this Oct 15, 2018

@massive

This comment has been minimized.

Copy link
Contributor Author

massive commented Oct 15, 2018

@kares Thanks for your input. I agree, the rounding part is a subpar solution.

However, while using longs fixes the problem with addition, it's not enough to fix the problems with subtraction. For instance, within the provided spec, the result for val in op_plus_numeric will be -864.0000000000001. The imprecision of the value will flow through the whole calculation causing odd effects (e.g. (dt - (dt - 0.0001)).to_f => 0.0001 but (dt - (dt - 0.00001)).to_f => -0.9999899884258123.

I'm not quite yet there to understand how and where to best deal with the problem though.

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 15, 2018

yeah having floats around is always problematic, think I have some ideas on how to proceed.
we should be able to somehow detect the "over-flowing" sub-traction case you mentioned.

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 15, 2018

also as a work-around you should be able to run fine with JRuby 9.1 .. 9.2.1 release should be close thought

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 16, 2018

for the record, only the add part is to be considered a regression, the sub-tract doesn't work in 9.1 either:

DateTime#- is able to subtract sub-millisecond precision values FAILED
Expected (7599824371187713/759982437118771200000)
 to equal (1/100000)

... we do the calculation "right" in 9.2 ~ the same as in previous JRubies.

can not use the rounding as is since (as CI revealed) that introduces more problems, will try to port the Date#-calculation logic to semi-match MRI, its tricky since we're using JODA while MRI has raw structs.

kares added a commit that referenced this pull request Oct 16, 2018

[fix] avoid DateTime addition int overflows
spec-ed DateTime#- part still failing, for now

adjusted from GH-5369

kares added a commit that referenced this pull request Oct 16, 2018

kares added a commit that referenced this pull request Oct 16, 2018

kares added a commit that referenced this pull request Oct 16, 2018

kares added a commit that referenced this pull request Oct 16, 2018

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 16, 2018

pushed a modified commit (with the long fix since its a regression) into master: bf2a83c ... so this doesn't hold up for long. tagged the failing subtract specs, might push a fix for that later - have one but it doesn't scale beyond 5 decimal places.

kares added a commit that referenced this pull request Oct 16, 2018

[fix] handle date + float with special care
so that 'precision' doesn't get lost on `(date + float) - date`
... still mostly just a work-around and does not behave as MRI

e.g. `date - 0.00001` passes but `date - 0.000001` doesn't
caused by having a different store and thus calculation algorithm

anyway at least we're now closer and GH-5369 should pass fine
@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 16, 2018

going to close this one, the provided specs are now passing ... there's still more work to do.
guess we'll track forward as users report, unless some of use decides to pro-actively work on it :)

@kares kares closed this Oct 16, 2018

eregon added a commit to ruby/spec that referenced this pull request Oct 27, 2018

[fix] avoid DateTime addition int overflows
spec-ed DateTime#- part still failing, for now

adjusted from jruby/jruby#5369
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.