deephaven / deephaven-core

Deephaven Community Core
Other
257 stars 80 forks source link

Modernize time APIs #528

Closed rcaudy closed 1 year ago

rcaudy commented 3 years ago

We ought to make our time representation friendlier for interoperability with other libraries.

It seems clear that the most appropriate solution for types is to migrate to the appropriate java.time classes:

  1. DBDateTime to Instant
  2. DBPeriod to Duration or Period depending on the use-case
  3. DBTimeZone to ZoneId. To aid in formula/condition brevity and encourage constant object re-use, we should keep DBTimeZone around as a class holding static members for each of its current enum constants, and give it a new name like TimeZones.

We'll need to update our library code accordingly:

  1. Functionality that is duplicative of the built-in library should be removed from our function libraries.
  2. Function libraries should be updated to support operator overloading using the migrated types.
  3. All usage of the org.joda time library should be entirely discontinued, in favor of java.time alternatives.

I think we should also strongly consider migrating away from io.deephaven.util.calendar.Calendar and towars java.util.Calendar, or at the very least renaming our interface to avoid confusion for external users. The Deephaven Calendar and BusinessCalendar APIs are likely in need of a careful review, at a minimum.

Lastly, we should not change our internal primitive representation for Instant. That is, we should continue to use long number of nanoseconds since the epoch, and convert from/to the this internal format when boxing/unboxing.

devinrsmith commented 3 years ago

So, I don't think that DBPeriod can conditionally be a Duration or Period - we need to choose one. If we need the other, we'll need to use that other type.

rcaudy commented 3 years ago

So, I don't think that DBPeriod can conditionally be a Duration or Period - we need to choose one. If we need the other, we'll need to use that other type.

The intent of my statement was that we should replace it with the appropriate type. In general, it's a wrapper around a org.joda.time.Period right now, and that appears to make it equivalent to a java.time.Period in intent/usage.

That said, I wouldn't be surprised if we were using it where we really wanted something more like a java.time.Duration in some cases.

The function library methods for operator overloading should probably just support plus(java.time.Instant, java.time.temporal.TemporalAmount) and minus(java.time.Instant, java.time.temporal.TemporalAmount).

chipkent commented 1 year ago

Already implemented.