Initial timezone support

[billylanchantin]

Description

Adds timezone support.

datetimes_with_timezones =
  ~U[2024-01-01T13:00:00.000000Z]
  |> DateTime.shift_zone!("America/New_York")
  |> List.wrap()

series = Explorer.Series.from_list(datetimes_with_timezones)
# #Explorer.Series<
#   Polars[1]
#   datetime[μs, America/New_York] [2024-01-01 07:00:00.000000-05:00 EST America/New_York]
# >

series.dtype
# {:datetime, :microsecond, "America/New_York"}

Explorer.Series.to_list(series)
# [#DateTime<2024-01-01 07:00:00.000000-05:00 EST America/New_York>]

Dtype changes

New dtype:

• {:datetime, precision, time_zone}

And the old naive datetime dtype has become:

• {:naive_datetime, precision}

See discussion below.

To-do

• Support %DateTime{} literals in macros
• Backward-compatibility: alias {:datetime, _} for {:naive_datetime, _}
  • Decided not to

[billylanchantin]

The main issue is that the new {:datetime, _, time_zone} dtype can't be easily enumerated in code since time_zone can be any valid time-zone string.

Before we were able to list literally every non-recursive dtype. That's no longer possible.

Also, as discussed on slack, I'll try to make {:datetime, _} be a valid alias for the new {:naive_datetime, _} dtype, possibly with a deprecation warning?

[billylanchantin]

My attempt to handle the fact that we can't list all the dtypes easily anymore. Open to ideas on this.

[philss]

The simplification makes sense to me 👍

[josevalim]

Also, as discussed on slack, I'll try to make {:datetime, _} be a valid alias for the new {:naive_datetime, _} dtype, possibly with a deprecation warning?

In my opinion it is fine to nuke it. It is a very easy change for folks to update in their own apps. Alternatively, keep it as a shortcut for {:datetime

[billylanchantin]

@philss (or anyone!) I've hit the limit of my ability to cargo cult Rust code 😭.

The ExDateTime struct currently looks like this:

#[derive(NifStruct, Copy, Clone, Debug)]
#[module = "DateTime"]
pub struct ExDateTime<'a> {
    // ...
    pub time_zone: &'a str,
    // ...
    pub zone_abbr: &'a str,
}

I need to derive these fields from the DateTime object, so they're not known at compile time. AFAICT I've got two options: they can have type &'a str or String. I can implement this if they have type String, but then we can no longer #[derive(Copy)]. So I've tried to implement it with &'a str, but I haven't figured out how yet.

Any pointers would be appreciated!

[philss]

Oh man, I really don't know how to solve this easily. Sorry :/
Do we need to support Copy? I would try to use String if we don't need this.

[billylanchantin]

Do we need to support Copy?

I dunno! I don't know what a lot of this code does... 😅

I'm gonna switch to String. If we end up needing Copy for some reason we can try a different approach.

[billylanchantin]

8f5be24

Ok, so we did end up needing Copy because of how I wrote s_from_list_datetime. Specifically, this breaks:

val: Vec<Option<ExDateTime>>
// ...
val.iter()
    .map(|dt| dt.map(|dt| dt.into()))
//            ^^^^^^^^^^^^^^^^^^^^^^ this implicitly requires Copy
    .collect::<Vec<Option<i64>>>(),

I've reverted to &'a str for now since all the tests pass. I suspect something we try with %DateTime{} will eventually break because of this, but I say let's address it then.

Thoughts?

[philss]

No thoughts for now :/
Let's go with this version, and we improve in the future :D

[philss]

The simplification makes sense to me 👍

[philss]

Can we guarantee that the timezones are the same between Elixir and Rust?

[billylanchantin]

Great call out! No, largely because Elixir's Calendar module is configurable. This is something which I think we'll want to be very clear about in the docs.

It also presents a validation challenge. As written, we aren't able to check that a time-zone string is valid on the Elixir side. We essentially have to assume it's fine and wait for Rust to tell us it didn't work.

[philss]

No problem. Yeah, I agree that if we document that, this is fine :)

[philss]

Oh man, I really don't know how to solve this easily. Sorry :/
Do we need to support Copy? I would try to use String if we don't need this.

[philss]

This is a great addition! 🚀

[billylanchantin]

Decided to go with what José suggested and nuke :datetime in favor of :naive_datetime. We can add :datetime back in as an alias before the next release if the change seems too disruptive.