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

Overflow calculating large periods #229

Closed
GoogleCodeExporter opened this Issue Mar 15, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@GoogleCodeExporter

GoogleCodeExporter commented Mar 15, 2015

Can't calculate total number of days in the ISO calendar.

  var a = new LocalDate(CalendarSystem.Iso.MinYear, 1, 1);
  var b = new LocalDate(CalendarSystem.Iso.MaxYear, 12, 31);
  var period = Period.Between(a, b, PeriodUnits.Days);
  var days = period.Days;

System.OverflowException
Arithmetic operation resulted in an overflow.
   at NodaTime.Fields.FixedLengthPeriodField.GetInt64Difference(LocalInstant minuendInstant, LocalInstant subtrahendInstant)
   at NodaTime.Period.Between(LocalDateTime start, LocalDateTime end, PeriodUnits units)
   at NodaTime.Period.Between(LocalDate start, LocalDate end, PeriodUnits units)

Also tried with just C.E. dates.  Still to large to calculate.  We could either 
use a larger data type, or possibly perform the operation in smaller units 
depending on the period units specified, instead of just operating on the raw 
ticks.

Original issue reported on code.google.com by mj1856 on 26 Jul 2013 at 7:16

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

I'm not entirely surprised - it's a difference which is more than 
long.MaxValue. We can *try* using ulong instead, but it may well be awkward.

Was this just a matter of interest, or did you have a particular reason for 
this?

Original comment by jonathan.skeet on 26 Jul 2013 at 7:34

GoogleCodeExporter commented Mar 15, 2015

I'm not entirely surprised - it's a difference which is more than 
long.MaxValue. We can *try* using ulong instead, but it may well be awkward.

Was this just a matter of interest, or did you have a particular reason for 
this?

Original comment by jonathan.skeet on 26 Jul 2013 at 7:34

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

Background - I'm working on SQLCLR integration again.  It's going very well, I 
have much of the previous issues resolved.  I'm now trying to get UDTs working 
for all of the public NodaTime types.  So, for example, you can actually create 
a column of type "localdate", which is a SQLCLR type "SqlLocalDate", which 
encapsulates a regular NodaTime LocalDate.

SQL's own "date" type supports values from years 1-9999, using 3 bytes.  These 
represent the number of whole days since 0001-01-01.  There are some downsides 
to using "date".  There is no CLR equivalent (only SqlDateTime exists, SqlDate 
is absent).  And it can't represent B.C. or far-future dates.

If I can get this working, then it solves problems such as this one: 
http://stackoverflow.com/q/5058709/634824

My UDT will need 4 bytes to cover the whole range supported by the ISO calendar 
in NodaTime.  But I would like to align years 1-9999 with the same values used 
by the SQL date type. That way it is bit-convertible within SQL, at least over 
that range.

So the problem is, I need to calculate days from 0001-01-01, negative or 
positive, for the entire supported range.  Right now it only works until 
Period.Between overflows.

Original comment by mj1856 on 26 Jul 2013 at 7:39

GoogleCodeExporter commented Mar 15, 2015

Background - I'm working on SQLCLR integration again.  It's going very well, I 
have much of the previous issues resolved.  I'm now trying to get UDTs working 
for all of the public NodaTime types.  So, for example, you can actually create 
a column of type "localdate", which is a SQLCLR type "SqlLocalDate", which 
encapsulates a regular NodaTime LocalDate.

SQL's own "date" type supports values from years 1-9999, using 3 bytes.  These 
represent the number of whole days since 0001-01-01.  There are some downsides 
to using "date".  There is no CLR equivalent (only SqlDateTime exists, SqlDate 
is absent).  And it can't represent B.C. or far-future dates.

If I can get this working, then it solves problems such as this one: 
http://stackoverflow.com/q/5058709/634824

My UDT will need 4 bytes to cover the whole range supported by the ISO calendar 
in NodaTime.  But I would like to align years 1-9999 with the same values used 
by the SQL date type. That way it is bit-convertible within SQL, at least over 
that range.

So the problem is, I need to calculate days from 0001-01-01, negative or 
positive, for the entire supported range.  Right now it only works until 
Period.Between overflows.

Original comment by mj1856 on 26 Jul 2013 at 7:39

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

Makes sense. I'm working on it now. I'm not expecting to be able to handle the 
number of *ticks* between the start and end of time, as that's more than 
long.MaxValue - but everything else should work.

I've got Subtract working - I just need to handle Add properly now...
(That's just in FixedDurationPeriodField; we'll have to see if months and years 
actually work...)

Original comment by jonathan.skeet on 26 Jul 2013 at 7:52

GoogleCodeExporter commented Mar 15, 2015

Makes sense. I'm working on it now. I'm not expecting to be able to handle the 
number of *ticks* between the start and end of time, as that's more than 
long.MaxValue - but everything else should work.

I've got Subtract working - I just need to handle Add properly now...
(That's just in FixedDurationPeriodField; we'll have to see if months and years 
actually work...)

Original comment by jonathan.skeet on 26 Jul 2013 at 7:52

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

This should be fixed in revision 1c63b9fb4025. Could you give it a try, Matt?

Original comment by jonathan.skeet on 26 Jul 2013 at 9:45

  • Changed state: Fixed

GoogleCodeExporter commented Mar 15, 2015

This should be fixed in revision 1c63b9fb4025. Could you give it a try, Matt?

Original comment by jonathan.skeet on 26 Jul 2013 at 9:45

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

Original comment by malcolm.rowe on 26 Jul 2013 at 10:02

  • Added labels: Milestone-1.2.0

GoogleCodeExporter commented Mar 15, 2015

Original comment by malcolm.rowe on 26 Jul 2013 at 10:02

  • Added labels: Milestone-1.2.0
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

Yes, it appears to work now.  Thanks!

Original comment by mj1856 on 26 Jul 2013 at 11:43

GoogleCodeExporter commented Mar 15, 2015

Yes, it appears to work now.  Thanks!

Original comment by mj1856 on 26 Jul 2013 at 11:43

@malcolmr malcolmr added the bug label Mar 15, 2015

@malcolmr malcolmr modified the milestone: 1.2.0 Mar 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment