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

In DateTime.jd, some Rational numbers cause RangeError (bignum too big to convert into `long') #5791

Closed
guyboertje opened this issue Jul 15, 2019 · 7 comments

Comments

@guyboertje
Copy link

commented Jul 15, 2019

I am getting this error in my rspec runs in the logstash-input-jdbc.
The Sequel gem (5.22.0) is using tzinfo (2.0.0) to generate DateTime instances when a timezone conversion is being done between the database timezone and the LS application timezone. In some cases a rational number that has very large numerator/denominator combinations is supplied to the jd class method on the JRuby DateTime class. This breaks with a RangeError (bignum too big to convert into `long'). It is easy to reproduce in IRB.

guy at Elastics-MacBook-Pro in ~/elastic/logstash-input-jdbc on jdk11-class-loading*
$ ruby -v
jruby 9.2.6.0 (2.5.3) 2019-02-11 15ba00b OpenJDK 64-Bit Server VM 11.0.2+9 on 11.0.2+9 +jit [darwin-x86_64]
guy at Elastics-MacBook-Pro in ~/elastic/logstash-input-jdbc on jdk11-class-loading*
$ bundle console
irb(main):001:0> rat = Rational("106143484200006057997/43200000000000")
=> (106143484200006057997/43200000000000)
irb(main):002:0> require 'date'
=> true
irb(main):003:0> dt = DateTime.jd(rat)
Traceback (most recent call last):
       16: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/friendly_errors.rb:124:in `with_friendly_errors'
       15: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/exe/bundle:30:in `block in <main>'
       14: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/cli.rb:18:in `start'
       13: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
       12: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/cli.rb:27:in `dispatch'
       11: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
       10: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
        9: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
        8: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/cli.rb:496:in `console'
        7: from /Users/guy/.rbenv/versions/jruby-9.2.6.0/lib/ruby/gems/shared/gems/bundler-2.0.2/lib/bundler/cli/console.rb:19:in `run'
        6: from org/jruby/RubyKernel.java:1179:in `catch'
        5: from org/jruby/RubyKernel.java:1179:in `catch'
        4: from org/jruby/RubyKernel.java:1411:in `loop'
        3: from org/jruby/RubyKernel.java:1047:in `eval'
        2: from (irb):3:in `evaluate'
        1: from org/jruby/ext/date/RubyDateTime.java:336:in `jd'
RangeError (bignum too big to convert into `long')
@kares

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

this is an internal limitation of Date/DateTime as it is currently implemented.
of course it could be improved but I am not sure how much changes it would need + its rarely needed.
in this case we could do 'rounding' of 106143484200006057997/43200000000000 (pbly not so smart).
are you sure that spec is testing something realistic, if not maybe we could just revisit that?

@guyboertje

This comment has been minimized.

Copy link
Author

commented Jul 15, 2019

IMO the internal limitation must be fixed. It does not happen in Ruby 2.4.0.

The specs are loading SQL Timestamps from a local JavaDB to test that the timezones are correctly converted. They are verifying a real world use case and are not calling Sequel -> TzInfo -> DateTime directly, rather, the specs set up the DB records and call plugin code to read the records and create Logstash events.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

The "limitation" in more explicit terms is simply that DateTime needs to get a long, right?

This particular error does not appear to be caused directly by that limitation; rather, when resolving a Rational time for the jd logic, we attempt to do it with two long values. A short term fix for this seems simple enough: use the Bignums directly to calculate a quotient and then cast that to long. This would lose some precision, but we can only put so much precision in Java's long-based time APIs.

I think this is better than failing completely as we do now.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

The logic in question is here:

static long getDay(ThreadContext context, IRubyObject day, final long[] rest) {
long d = day.convertToInteger().getLongValue();
if (!(day instanceof RubyInteger) && day instanceof RubyNumeric) { // Rational|Float
RubyRational rat = ((RubyNumeric) day).convertToRational();
long num = rat.getNumerator().getLongValue();
long den = rat.getDenominator().getLongValue();
rest[0] = (num - d * den); rest[1] = den;
}
return d;
}

This logic needs to calculate a quotient without first coercing those values to long. I could not come up with a five minute fix because of the use of the long[2] rest parameter that demands the denominator in a long (which we can do for this case, but not in general). There are a few other consumers of this logic that would need to be updated to not require long values.

@headius headius added this to the JRuby 9.2.8.0 milestone Jul 15, 2019

@headius

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

I agree this should be fixed.

@guyboertje

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

@headius
Thanks.

I tried to see if I could wrangle my specs to not use timestamps that exhibit the fault but it seems that tzinfo is using the Unix Epoch as a datum to determine the local tz offset from UTC when it converting other timestamps from one timezone to another. Meaning I don't think I can avoid it without monkey patching tzinfo.

kares added a commit to kares/jruby that referenced this issue Jul 31, 2019

[fix] handle Bignum DateTime.jd fraction
uncommon but MRI handles it as reported on jrubyGH-5791
a quick fix, might need more MRI compatibility work

kares added a commit to kares/jruby that referenced this issue Aug 1, 2019

[fix] handle Bignum DateTime.jd fraction
uncommon but MRI handles it as reported on jrubyGH-5791
a quick fix, will need more MRI compatibility work

kares added a commit that referenced this issue Aug 1, 2019

[fix] handle Bignum DateTime.jd fraction
uncommon but MRI handles it as reported on GH-5791
a quick fix, will need more MRI compatibility work
@kares

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

the problematic case Rational("106143484200006057997/43200000000000") should now work.
there's still issues left regarding rounding - its a wild shot but at least we wont error out and possibly only miss second's fraction

@kares kares closed this Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.