[C++] interval_between(timestamptz, timestamptz) -> struct kernel

[asfimport]

Given two timestamps that each have time zones this function should return an interval from the first timestamp to the second timestamp according to the following rules (postgres rules):

• 1 day is 24 physical hours (which may not exactly equal 1 calendar day).

• Intervals returned will never contain a months/years field

  Examples:

  interval_between('2021-03-14 00:00:00 America/Denver', '2021-03-15 00:00:00 America/Denver') => { "hours": 23 }
  interval_between('2021-03-14 00:00:00 UTC', '2021-03-15 00:00:00 UTC') => { "days": 1}
  interval_between('2021-03-14 00:00:00 UTC', '2020-03-14 00:00:00 UTC') => { "days": 365}

  If the first timestamp is larger than the second timestamp then the interval will be negative.

Reporter: Weston Pace / @westonpace
Assignee: Rok Mihevc / @rok
Watchers: Rok Mihevc / @rok

PRs and other links:

• GitHub Pull Request #12477

_{Note: This issue was originally created as ARROW-14108. Please see the migration documentation for further details.}

[asfimport]

David Li / @lidavidm:
ARROW-13548 implements this for the actual month, day-time, and month-day-nano intervals. I suppose this falls under day-time except the resolution may not be fine enough?

[asfimport]

David Li / @lidavidm:
Oh, on a closer reading, this is different, given the days are not calendar days.

[asfimport]

Weston Pace / @westonpace:
Right, this also returns intervals (not integers) and it can return, for example,

{ "days": 1, "hours": 10, "minutes": 5 }

[asfimport]

David Li / @lidavidm:
(To be clear, ARROW-13548 implements kernels returning both integers and the interval types)

I suppose we'd need this to be 1:1 compatible with Postgres, but it feels like a bit of a waste having just added a new interval type (month-day-nano) which IIRC from the discussions, was supposed to be as general as possible.

[asfimport]

Weston Pace / @westonpace:
I was basing my understanding of ARROW-13548 on SQL's DATEDIFF (which requires you to specify a time unit and only returns an integer). I see now the month_day_nano between which is very cool.

In a certain sense this is not needed for full postgres compatibility. A user that want's that behavior can just grab a duration from two timestamptz objects and do the math themselves. However, it would make postgres compatibility much easier.

That being said, isn't this just another flavor of the extraction kernels? It can be day_nano_between.

It's hard to say one is more canonical than the other when someone could also presumably ask for year_month_day_nano_between.

[asfimport]

David Li / @lidavidm:
That's fair, I'm more lamenting all the permutations here :)

That said, presuming we'd then turn around and implement date/time arithmetic with these "interval" structs, we'd effectively be defining additional, custom interval types (which is fine, just something to note).

[asfimport]

Rok Mihevc / @rok:
I'm not sure how to read this - are we looking to implement a new interval type here or using one of the existing ones? e.g. DayTimeIntervalType
Would a struct serve better? @lidavidm @westonpace

[asfimport]

David Li / @lidavidm:
I'm not sure if we strictly need this in the first place, but it seems this would return a struct, yes. (Though, it's pretty much redundant with using something like seconds_between and just dividing out the hours yourself.)

[asfimport]

Rok Mihevc / @rok:
Yeah, that makes sense.

[asfimport]

Weston Pace / @westonpace:
I agree it is low priority. We could probably sit on it unless it comes up wrt Substrait compatibility though it seems it isn't included in the function set just yet: https://github.com/substrait-io/substrait/blob/main/extensions/functions_datetime.yaml

[asfimport]

Weston Pace / @westonpace:
Also, a struct is fine.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
I find it a bit questionable that we need a kernel to return a day/hour struct, while we already have a kernel that returns the day_time interval, from which you can easily extract the day and milliseconds (and thus day and computed hour) if you want those.
(and also a kernel that returns month_day_nano interval between, although that is harder to convert into a day/hour combo)

Or at least, if we add a convenience to directly get day/hour struct, could it be a simple wrapper around day_time_interval_between? @rok Or would that give different results than the custom implementation you did now?

[asfimport]

Rok Mihevc / @rok:
@jorisvandenbossche Thanks for reviewing! I'm not sure how this will get consumed in the end and how much convenience this kernel brings. Since the output is a struct I don't really see much of possibility for reuse, but I might (very likely) just be missing it.

Perhaps an elegant way would be to add cast(DayTimeBetween) -> dayhours_between and handle this via dispatchBest. I'm not sure how much overhead that brings.

@westonpace could you quantify the convenience gained by having this kernel?

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:

Since the output is a struct I don't really see much of possibility for reuse, but I might (very likely) just be missing it.

This might not yet be implemented, but based on my limited understanding of the DayTimeInterval type, you should be able to extract the days and milliseconds from such values. If that is implemented, you could in theory have a kernel that uses day_time_interval_between, then extracts the days / milliseconds as separate fields and put them in a struct array. Yes, that will be less efficient (as you have the intermediate interval array). But unless there is a strong demand for an optimal "day_hour_between", I would start with reusing the existing interval kernel.

(note, this are a bit naive comments, not really knowing the use case for this, and just from looking at your PR and seeing that this adds quite some custom logic for something that basically already exists elsewhere)

[asfimport]

Rok Mihevc / @rok:
@jorisvandenbossche I agree that we could use day_time_interval_between as an intermediate step.

The reason for a lot of boiler plate here is lack (as far as I understand) of templates for kernels with StructArray output. Even if doing cast(day_hours, day_time_interval_between(ts_arr0, ts_arr1)) we would need some of it. Although there would be less due to that cast being a unary kernel. Perhaps we could even abstract out a template and reuse it on ISOCalendar and YearMonthDay kernels.

Either way I would be perhaps be useful to hear about Westons usecase. @westonpace