Add duration datatypes #683

billylanchantin · 2023-08-20T21:27:18Z

Description

This is a WIP. I'm hoping for some feedback before I get too into the weeds!

EDIT: Rewrote this section for clarity.

Adds the following dtypes:

{:duration, :nanosecond}
{:duration, :microsecond}
{:duration, :millisecond}

Adds support for some basic operations with those dtypes:

Explorer.Series.from_list/2
Explorer.Series.{add/2, subtract/2, multiply/2, divide/2}
- These now work in combination with the {:datetime, *} dtypes

In principle we could also support most 1-arity functions which accept :integer since durations are :integer under the hood. But I wanted to stop here to see if this is roughly what the team had in mind.

Examples

Adapted from test/duration_test.exs:

`Series.add/2`

iex(1)> eleven = Series.from_list([~N[2023-08-20 11:00:00.0000000]])
iex(2)> one_hour = Series.from_list([3600 * 1_000], dtype: {:duration, :millisecond})

#Explorer.Series<
  Polars[1]
  duration[ms] [3600000]
>

iex(3)> sum = Series.add(eleven, one_hour)

#Explorer.Series<
  Polars[1]
  datetime[μs] [2023-08-20 12:00:00.000000]
>

Click to show/hide more examples

`Series.subtract/2`

iex(1)> eleven = Series.from_list([~N[2023-08-20 11:00:00.0000000]])
iex(2)> twelve = Series.from_list([~N[2023-08-20 12:00:00.0000000]])
iex(3)> one_hour = Series.subtract(twelve, eleven)

#Explorer.Series<
  Polars[1]
  duration[μs] [3600000000]
>

`DataFrame.mutate/2`

iex(1)> df = DF.new(eleven: [~N[2023-08-20 12:00:00.0000000]], twelve: [~N[2023-08-20 11:00:00.0000000]])
iex(2)> df_with_diff = DF.mutate(df, diff: twelve - eleven)

#Explorer.DataFrame<
  Polars[1 x 3]
  eleven datetime[μs] [2023-08-20 11:00:00.000000]
  twelve datetime[μs] [2023-08-20 12:00:00.000000]
  diff duration[μs] [3600000000]
>

Related Work

Overview of approach

EDIT: Rewrote this section for clarity.

The main idea is to rely on polars native support for durations. For example:

https://www.rustexplorer.com/b/yzh47r

// Excerpt from example
// ...
let dt1 = Series::new("datetime1", dt1_values);
let dt2 = Series::new("datetime2", dt2_values);
let diff = Series::new("diff", &dt2 - &dt1);
println!("{:?}", diff);

shape: (2,)
Series: 'diff' [duration[ms]]
[
	2d
	3d
]

To enable this, I've created explicit duration dtypes on the Elixir side and mappings to/from the Rust side which mirror the equivalent ones for datetime:

s_from_list_datetime
s_from_list_duration <-- new
datetime_series_to_list
duration_series_to_list <-- new

Then on the Elixir side, I'm doing a bunch of case work to make sure the Explorer.Series.{add,subtract,multiply,divide} functions recognize situations where a duration is involved and let that operation through. Before they'd only allow something like (pseudocode):

:integer - :integer

They now allow things like:

{:datetime, :microsecond} - {:duration, :nanosecond}

This required that I figure out what situations are allowed ahead of time. I took a stab at what I thought the allowed operations should be:

Click to show/hide

Operation	Left	Right	Output
`:add`	`%NaiveDateTime{}`	`{:duration, r}`	`{:datetime, r}`
`:add`	`{:duration, l}`	`%NaiveDateTime{}`	`{:datetime, l}`
`:add`	`{:datetime, l}`	`{:duration, r}`	`{:datetime, higher(l, r)}`
`:add`	`{:duration, l}`	`{:datetime, r}`	`{:datetime, higher(l, r)}`
`:add`	`{:duration, l}`	`{:duration, r}`	`{:duration, higher(l, r)}`
`:subtract`	`%NaiveDateTime{}`	`{:datetime, r}`	`{:duration, r}`
`:subtract`	`{:datetime, l}`	`%NaiveDateTime{}`	`{:duration, l}`
`:subtract`	`%NaiveDateTime{}`	`{:duration, r}`	`{:datetime, r}`
`:subtract`	`{:datetime, l}`	`{:datetime, r}`	`{:duration, higher(l, r)}`
`:subtract`	`{:datetime, l}`	`{:duration, r}`	`{:datetime, higher(l, r)}`
`:subtract`	`{:duration, l}`	`{:duration, r}`	`{:duration, higher(l, r)}`
`:multiply`	`{:duration, l}`	`:integer`	`{:duration, l}`
`:multiply`	`:integer`	`{:duration, r}`	`{:duration, r}`
`:divide`	`{:duration, l}`	`:integer`	`{:duration, l}`

where higher/2 selects the higher precision assuming :nanosecond > :microsecond > :millisecond.

We can edit this as needed.

Notably, durations are currently getting mapped to bare integers on the Elixir side. E.g.

eleven = Series.from_list([~N[2023-08-20 11:00:00.0000000]])
twelve = Series.from_list([~N[2023-08-20 12:00:00.0000000]])
diff = Series.subtract(twelve, eleven)

diff |> Series.to_list() |> IO.inspect() #=> [3_600_000_000]

Since there's no native duration in Elixir, I think that's the best we can do with this approach unless we create our own %Explorer.Duration{}.

Open questions

Are we okay with the durations being bare integers on the elixir side?
- No, we're going with %Explorer.Duration{}
Should we support durations for Time and Date? I only implemented comparing NaiveDateTimes. Doing so will be a bit tricky.
- No, we'll support date - date in a later PR and won't support time - time at all.
How do we feel about picking the highest precision? (See the operation examples)
- 👍

To-do

philss

Hey @billylanchantin, this is looking awesome!

I dropped some comments, but I think this is a good start!

lib/explorer/backend/lazy_series.ex

philss · 2023-08-22T04:48:34Z

native/explorer/src/encoding.rs

@@ -467,6 +479,7 @@ pub fn term_from_value<'b>(v: AnyValue, env: Env<'b>) -> Result<Term<'b>, Explor
        AnyValue::Date(v) => encode_date(v, env),
        AnyValue::Time(v) => encode_time(v, env),
        AnyValue::Datetime(v, time_unit, None) => encode_datetime(v, time_unit, env),
+        AnyValue::Duration(v, _time_unit) => Ok(Some(v).encode(env)),


I wonder if we could have a custom type (struct) for this at the Elixir side, and return the time unit with it. Thoughts?

I actually started down that road before I went with integers for simplicity. I still think that may be the right choice, but I wanted to hear y'all's thoughts first.

In particular, bare integers can't capture the units and so don't survive (de)serialization. E.g. this doesn't work:

import Explorer.Series dt1 = from_list([~N[2023-01-01 00:00:00]]) dt2 = from_list([~N[2023-01-02 00:00:00]]) dt2 |> subtract(dt1) |> to_list |> from_list |> add(dt1)

You get

** (ArgumentError) cannot invoke Explorer.Series.add/2 with mismatched dtypes: :integer and {:datetime, :millisecond}

If we had a custom %Explorer.Duration{} or similar, the above would work just fine. The downside is that Explorer would need to internally represent durations. That would increase the maintenance burden and (possibly?) make it harder to implement custom backends.

I still think it may be worth it but that's the tradeoff IMO.

I think it is fine to keep it as integer, but we need to choose a unique representation, which would most likely be nanoseconds?

Clarification: I was saying a %Explorer.Duration{} struct might be the way to go despite the overhead. (Sorry about my wording.)

Representing durations as integers in Elixir forces the programmer to always pass along type hints whenever a duration is initially created. That may be cumbersome if you're doing multiple transformations involving a duration column. This is unlike datetime where, once you've created the initial series, you can do:

datetime = from_list([1649883642 * 1_000 * 1_000], dtype: {:datetime, :millisecond}) #=> #Explorer.Series< #=> Polars[1] #=> datetime[μs] [54252-09-24 11:40:00.000000] #=> >

then

datetime |> to_list |> from_list #=> #Explorer.Series< #=> Polars[1] #=> datetime[μs] [54252-09-24 11:40:00.000000]

which preserves the precision without having to re-pass in the :dtype arg.

However, it looks like we're already in this situation WRT binary:

binary = from_list([<<228, 146, 51>>, "Elixir"], dtype: :binary) #=> #Explorer.Series< #=> Polars[2] #=> binary [<<228, 146, 51>>, "Elixir"] #=> >

Here you have that same problem (actually it's even worse because we raise):

binary |> to_list |> from_list #=> ** (ArgumentError) argument error

So I think my objection isn't as strong.

If we do stay with integers, then I think normalizing on nanosecond precision is the way to go. I'm gonna assume that's the plan for now.

There is an issue in normalizing to nanoseconds: a very large value will overflow 64 bits, which is fine for Elixir, but it will lead issues when converting back to Explorer. So I think you are right, Explorer.Duration is best way to go indeed.

How would we implement it? Do we keep two fields? Value and precision? Elixir's Calendar types represent the precision as a number (from 0 to 6) we could do the same here and represent it as a number from 0 to 9, but given it will be different anyway, the precision as one of :millisecond | :microsecond | :nanosecond is fine.

Thoughts?

Something to add: in Elixir, the precision is about the representation. If we say the datetime is a single digit, then it is a single digit precision. In here, the precision is about low-level storage (i.e. the biggest/smallest number we can represent). Given they are different, let's go with the precision being an atom (the time unit).

Yeah I was thinking exactly that:

%Explorer.Duration{ value: integer, precision: :millisecond | :microsecond | :nanosecond }

We may find we want more fields down the road of course. But right now this struct only needs to hold on to enough information to make the mapping into polars unambiguous.

Addressed by: 98a8ff7

lib/explorer/backend/lazy_series.ex

josevalim · 2023-08-22T07:43:31Z

lib/explorer/series.ex

@@ -2201,14 +2215,13 @@ defmodule Explorer.Series do
  @spec skew(series :: Series.t(), opts :: Keyword.t()) :: float() | non_finite() | nil
  def skew(series, opts \\ [])

-  def skew(%Series{dtype: dtype} = series, opts)
-      when is_numeric_or_date_dtype(dtype) do
+  # BILLY-NOTE: Bug? They're checking for numeric-or-temporal but only allowing numeric


If it is simpler, let's only allow numeric here for now.

josevalim · 2023-08-22T07:43:54Z

lib/explorer/series.ex

+          right :: Series.t() | number() | NaiveDateTime.t()
+        ) :: Series.t()
+  def add(%NaiveDateTime{} = left, %Series{dtype: {:duration, timeunit}} = right),
+    do: apply_series_list(:add, [left, right]) |> cast({:datetime, timeunit})


Just a nitpick: I am not sure if we should cast here. If we cast here, we are assuming that the backend does not return something with the correct dtype. I think that's a backend logic: we say what is the expected dtype and the backend is responsible for dealing with it. If we remove the cast, what will happen? Do we get an integer or duration or datetime back?

If we get a different behaviour than polars, could this mean that we are actually not casting NaiveDateTime properly when sending it to Polars? Or are we choosing a different behaviour than Polars? If so, why?

The same rationale applies to cast in subtract, multiply, and divide.

If we remove the cast, what will happen? Do we get an integer or duration or datetime back?

The rationale was maintaining highest precision. If we do

{:datetime, :millisecond} + {:duration, :nanosecond}

I wanted to make sure we ended up with

{:datetime, :nanosecond}

as the return type. Otherwise we'd lose information. However, I'm now realizing that we've already lost information because of the way I've written it 🤦. I need to be casting before the call to polars like I did with the other clauses.

import Explorer.Series datetime = ~N[2023-01-01 00:00:00] duration = from_list([100], dtype: {:duration, :nanosecond}) # Current implementation: datetime |> add(duration) |> cast(:integer) |> to_list #=> [1672531200000000000] # What I actually wanted: [datetime] |> from_list(dtype: {:datetime, :nanosecond}) |> add(duration) |> cast(:integer) |> to_list #=> [1672531200000000100]

I'll rework the clauses where I'm post-casting.

Right, I would like to avoid the postcast in general. I also think we should consider converting the NaiveDateTime to a series in series.ex, so the backend does not have to handle it either.

There was a reason why we didn't do it, but i don't fully recall. I will investigate.

I double checked, I think this will be fine: if any of the arguments is a %NaiveDateTime{}, you can call from_list([dt], {:datetime, precision}). This means we won't need to deal with it in the backend.

Did this for add and subtract here: 2f77e76

Multiply and divide seem determined to get returned as integers. Will investigate what's going on tomorrow.

Ok I think I know what's going on. Polars seems to be treating scalar and element-wise multiplication differently.

https://www.rustexplorer.com/b/4cdukd

println!("{:?}", diff); // [1d] println!("{:?}", Series::new("prod1", &diff * 10)); // [10d] println!("{:?}", Series::new("prod2", (10).mul(&diff))); // [10d] println!("{:?}", Series::new("prod3", &ten * &diff)); // [864000000] println!("{:?}", Series::new("prod4", &diff * &ten)); // [864000000]

Unfortunately it's the element-wise multiplication that's returning integers. Also:

println!("{:?}", Series::new("prod4", &diff * &diff)); //=> mul operation not supported for dtypes `duration[ms]` and `duration[ms]`

So I think we either have to drop support or post-cast.

I wonder if this is a bug in polars. Seems like an odd choice if it's intentional, especially since pandas supports it:

import pandas as pd duration = pd.Series([pd.Timedelta(1, "d"), pd.Timedelta(2, "d")]) integer = pd.Series([10, 10]) print(duration * integer) #=> 0 10 days #=> 1 20 days #=> dtype: timedelta64[ns]

josevalim

Amazing work @billylanchantin! 💯

The direction is great, we added only some minor comments so we can go ahead with this.

Also note we need to add Series.to_iovec support. This should be similar to encoding datetime in encoding.rs, so I am hoping it is not a major addition. :)

lib/explorer/series.ex

Co-authored-by: José Valim <jose.valim@gmail.com>

lib/explorer/series.ex

josevalim · 2023-08-23T09:53:11Z

native/explorer/src/encoding.rs

@@ -493,6 +506,7 @@ pub fn list_from_series(s: ExSeries, env: Env) -> Result<Term, ExplorerError> {
        DataType::Date => date_series_to_list(&s, env),
        DataType::Time => time_series_to_list(&s, env),
        DataType::Datetime(time_unit, None) => datetime_series_to_list(&s, *time_unit, env),
+        DataType::Duration(time_unit) => duration_series_to_list(&s, *time_unit, env),


Just a note that whatever we decide above we need to mirror here. :)

Addressed by: 98a8ff7

josevalim · 2023-08-23T09:56:45Z

lib/explorer/backend/lazy_series.ex

+    end
+  end
+
+  defp resolve_numeric_temporal_dtype(op, left, %Series{dtype: dtype} = right) do


I think we can remove this clause, right? NaiveDateTimes will be normalized to series in series.ex, so no longer a concern here. :)

Yes I think so. Done: 98c2885

Seems like we can make a similar simplification to resolve_numeric_dtype/1 in that same module.

I have another PR for this, which I want to merge after this one to not mess up your code. :)

josevalim

I did another pass. I have added some minor notes, but it seems the major decisions have been taken, so we are on a solid path!

billylanchantin · 2023-08-23T13:05:00Z

@josevalim Awesome, thanks! I feel like we're in a pretty good spot too :)

One other major decision to be made: do we want to support date - date? I checked and polars does support it (and does not support time - time):

https://www.rustexplorer.com/b/cpbrw8

Co-authored-by: José Valim <jose.valim@gmail.com>

josevalim · 2023-08-23T13:28:02Z

Let’s add date-date in another PR so we pack this one sooner. :)

billylanchantin · 2023-08-23T19:34:03Z

test/explorer/series/duration_test.exs

+      # Each series displays its values as "[1ms]" as well as the correct precision.
+      assert inspect(ms) == """
+             #Explorer.Series<
+               Polars[1]
+               duration[ms] [1ms]
+             >\
+             """
+
+      assert inspect(us) == """
+             #Explorer.Series<
+               Polars[1]
+               duration[μs] [1ms]
+             >\
+             """


I took a pass at implementing the string-related protocols while I was adding the duration struct. Not having them broke one of my tests (plus I was tired of counting 0's). Feel free to 👍 / 👎 on the specifics. I mostly tried to copy what polars was doing. But for inspect I went with:

inspect(%Explorer.Duration{value: 1, precision: :millisecond}) #=> "Duration[1ms]"

I don't feel strongly about that choice, I just went with something.

lib/explorer/series.ex

josevalim · 2023-08-24T14:39:46Z

This is awesome @billylanchantin, thank you so much! 💚 💙 💜 💛 ❤️

This is complete as is and will unblock us on upcoming work. Feel free to send PRs for the time-time and the other missing pieces. :)

josevalim · 2023-08-24T14:54:03Z

Just one additional note. I removed the integer/duration operations from multiply/divide because we are not allowing integers on add/subtract, so I thought we should wait and tackle all of this at once. :)

josevalim · 2023-08-24T15:10:01Z

Another note: to support operations between duration and integers, I think you can change basic_numeric_operation directly. I have just pushed some changes to make that a bit easier too.

billylanchantin · 2023-08-24T15:21:32Z

Of course! I'm glad we could add this :) Thanks for all the feedback, @josevalim and @philss!

Feel free to send PRs for the time-time and the other missing pieces.

Will do! Not sure when exactly, but I've got a vested interest in this functionality. I'm working on a project that's attempting to predict the length of a duration.

I thought we should wait and tackle all of [the integer/duration stuff] at once. :)

👍

to support operations between duration and integers, I think you can change basic_numeric_operation directly.

Noted! Things I'll be keeping in mind:

Maintaining highest precision
Allowable types are operation-dependent, e.g.:
- duration + datetime is allowed
- duration - datetime is not
Polars is inconsistent with their types AFAICT

billylanchantin · 2023-08-24T15:34:11Z

native/explorer/cargo-toolchain.toml

+[toolchain]
+channel = "nightly-2023-06-23"


I forgot to call this out earlier. The most recent rust nightly didn't work for me and I had to use the specific version from the README. I've never really used rust before and IDK if what I did is a good practice. 🤷‍♂️

Just wanted to make sure this wasn't lost in the shuffle.

No problem! I think it's a good idea to keep this file. Polars is doing the same.
I think we can simplify our CI files to read from this, but I don't know how yet.

Thanks for pointing this out!

josevalim · 2023-08-24T15:51:41Z

I have just pushed a refactoring to basic_numeric_operation and the boolean mask now called cast_to_ordered_series which should make it easier to add operations between integers and durations.

billylanchantin added 13 commits August 19, 2023 22:49

newer nightly didn't work

112a4a5

get something working

aef40bd

some docs

976be80

begin casework

07c4203

remove old code

f6147dc

add ndt scalar clauses

5ccd946

add/sub/mul/div for lazy_series

e285d5a

fix doctests

0b8ea78

fix issue with resolving dtypes

a3c13a6

add initial tests

1ddac25

remove dead code

f0b9b90

remove more dead code

4b018e8

drop unnecessary map

c2848af

philss reviewed Aug 22, 2023

View reviewed changes

josevalim reviewed Aug 22, 2023

View reviewed changes

lib/explorer/backend/lazy_series.ex Show resolved Hide resolved

josevalim reviewed Aug 22, 2023

View reviewed changes

lib/explorer/series.ex Outdated Show resolved Hide resolved

josevalim mentioned this pull request Aug 22, 2023

Convert numeric values to series early #685

Merged

billylanchantin and others added 7 commits August 22, 2023 16:47

drop guard

91befdd

Co-authored-by: José Valim <jose.valim@gmail.com>

Merge branch 'main' into add-duration-datatypes

13c4553

raise when dividing by duration

743b306

disallow divide by duration

7bcef98

begin more exhaustive test suite

7e7b961

RuntimeError when dividing by duration

3357f90

use from_list instead of cast for add/subtract

2f77e76

josevalim reviewed Aug 23, 2023

View reviewed changes

lib/explorer/series.ex Outdated Show resolved Hide resolved

josevalim reviewed Aug 23, 2023

View reviewed changes

RuntimeError -> ArgumentError

f4363f6

Co-authored-by: José Valim <jose.valim@gmail.com>

billylanchantin added 5 commits August 23, 2023 09:48

fix test after error type change

936f36a

Series.to_iovec

f7009ce

missed this at some point

90ba273

first pass at duration struct

98a8ff7

example to_string/inspect for duration struct

c8afb37

billylanchantin commented Aug 23, 2023

View reviewed changes

billylanchantin and others added 2 commits August 23, 2023 16:33

drop unneeded function clause

98c2885

Merge branch 'main' into add-duration-datatypes

65cfdb5

josevalim reviewed Aug 24, 2023

View reviewed changes

lib/explorer/series.ex Outdated Show resolved Hide resolved

Update lib/explorer/series.ex

8738080

josevalim reviewed Aug 24, 2023

View reviewed changes

lib/explorer/series.ex Outdated Show resolved Hide resolved

Update lib/explorer/series.ex

34664eb

josevalim merged commit 6836f29 into elixir-explorer:main Aug 24, 2023
4 checks passed

billylanchantin commented Aug 24, 2023

View reviewed changes

billylanchantin deleted the add-duration-datatypes branch August 25, 2023 01:21

billylanchantin mentioned this pull request Aug 26, 2023

Additional temporal arithmetic #696

Merged

		[toolchain]
		channel = "nightly-2023-06-23"

Add duration datatypes #683

Add duration datatypes #683

Conversation

billylanchantin commented Aug 20, 2023 • edited Loading

Description

Examples

Series.add/2

Series.subtract/2

DataFrame.mutate/2

Related Work

Overview of approach

Open questions

To-do

philss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billylanchantin Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billylanchantin Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

billylanchantin commented Aug 23, 2023

josevalim commented Aug 23, 2023

Choose a reason for hiding this comment

josevalim commented Aug 24, 2023

josevalim commented Aug 24, 2023 • edited Loading

josevalim commented Aug 24, 2023

billylanchantin commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Aug 24, 2023

billylanchantin commented Aug 20, 2023 •

edited

Loading

`Series.add/2`

`Series.subtract/2`

`DataFrame.mutate/2`

billylanchantin Aug 23, 2023 •

edited

Loading

billylanchantin Aug 23, 2023 •

edited

Loading

josevalim commented Aug 24, 2023 •

edited

Loading