date_trunc panicked with message: called `Option::unwrap()` on a `None` value

[appletreeisyellow]

Describe the bug

date_trunc() panicked for some timestamps that are casted to a timezone other than UTC

For example, this query causes a panic:

SELECT date_trunc('week', arrow_cast(arrow_cast('2023-10-29T00:00:00Z', 'Timestamp(Nanosecond, Some("UTC"))'), 'Timestamp(Nanosecond, Some("Europe/Berlin"))'));

Even though a different timestamp works fine, e.g. 2023-10-29T02:00:00Z

To Reproduce

Run the above query in DataFusion CLI v34.0.0

❌2023-10-29T00:00:00Z panicked

$ datafusion-cli

DataFusion CLI v34.0.0
❯ SELECT date_trunc('week', arrow_cast(arrow_cast('2023-10-29T00:00:00Z', 'Timestamp(Nanosecond, Some("UTC"))'), 'Timestamp(Nanosecond, Some("Europe/Berlin"))'));
thread 'main' panicked at /private/tmp/datafusion-20231217-5103-4z8blm/arrow-datafusion-34.0.0/datafusion/physical-expr/src/datetime_expressions.rs:335:14:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

✅ 2023-10-29T02:00:00Z works

$ datafusion-cli

DataFusion CLI v34.0.0
❯ SELECT date_trunc('week', arrow_cast(arrow_cast('2023-10-29T02:00:00Z', 'Timestamp(Nanosecond, Some("UTC"))'), 'Timestamp(Nanosecond, Some("Europe/Berlin"))'));
+-------------------------------------------------------+
| date_trunc(Utf8("week"),Utf8("2023-10-29T02:00:00Z")) |
+-------------------------------------------------------+
| 2023-10-23T00:00:00+02:00                             |
+-------------------------------------------------------+
1 row in set. Query took 0.005 seconds.

Expected behavior

Handle the result correctly or return an error instead of panicking

Additional context

No response

[jayzhan211]

The issue is due to the ambiguous time between daylight saving time and standard time in chrono.

fn map_local<Tz: TimeZone, F>(dt: &DateTime<Tz>, mut f: F) -> Option<DateTime<Tz>>
where
    F: FnMut(NaiveDateTime) -> Option<NaiveDateTime>,
{
    f(dt.naive_local()).and_then(|datetime| dt.timezone().from_local_datetime(&datetime).single())
}

Return value of dt.timezone().from_local_datetime(&datetime)

Ambiguous(2023-10-29T02:00:00TzOffset { tz: Tz(Timezone(Europe/Berlin)), offset: +02:00 }, 2023-10-29T02:00:00TzOffset { tz: Tz(Timezone(Europe/Berlin)), offset: +01:00 })

https://github.com/chronotope/chrono/blob/5aaf74235778120b5984b46ced47478c1431d9a0/src/datetime/mod.rs#L693-L698

Therefore .single() return None in this case.

I think the we should convert it to NaiveDateTime to modify the time then convert back after.

https://github.com/apache/arrow-datafusion/blob/57e38fb16a9918fb16c2c50e2188cc22c0afe4ad/datafusion/physical-expr/src/datetime_expressions.rs#L234-L288

Ref

https://docs.rs/chrono/latest/chrono/struct.DateTime.html#method.with_second
https://docs.rs/chrono/latest/chrono/naive/struct.NaiveTime.html#method.with_second

[tustvold]

Would https://docs.rs/chrono/latest/chrono/offset/enum.LocalResult.html#method.earliest work instead? It would take the earlier of the ambiguous time, which would be consistent with the truncating behaviour of date_trunc. Converting to NaiveDateTime raises the risk of extremely subtle bugs that are likely hard to detect/interpret?

I'd definitely take a noisy failure, such as this, than silently incorrect behaviour.

Edit: this approach will still fail if the provided timestamp is invalid, and so we should probably change it to error instead of panic, but I think this is the correct behaviour. Truncating a timestamp that doesn't correspond to an actual instant in time, I think should error 😄

[mhilton]

Would https://docs.rs/chrono/latest/chrono/offset/enum.LocalResult.html#method.earliest work instead?

I don't think it would, at least not in all cases. If we take the two times:

• 2024-10-27 02:30:00 +0100 (CET)
• 2024-10-27 02:30:00 +0200 (CEST)

which are both valid in the Europe/Berlin timezone. Then using earliest in a date_trunc('hour', ...) function would cause the result for the latter time to be 2024-10-27 02:00:00 +0100 essentially truncating to the wrong hour.

My thought is that in cases where the result is Ambiguous then the value that has the same offset as the input time should be chosen.

Edit: this approach will still fail if the provided timestamp is invalid

Is it possible for an invalid input value to get as far as the date_trunc function, or would it be caught sooner in the processing?

[tustvold]

My thought is that in cases where the result is Ambiguous then the value that has the same offset as the input time should be chosen.

But what if the input doesn't have a fixed offset, i.e. some values are either side of DST 😅.

essentially truncating to the wrong hour.

Is it the wrong hour? If I input a timestamp of 2024-10-27 02:30:00 Europe/Berlin it would truncate to one of the possible representations of 2024-10-27 02:00:00 Europe/Berlin in UTC, the output is no more or less ambiguous than the input?

I guess we should probably see what postgres does.

Is it possible for an invalid input value to get as far as the date_trunc function, or would it be caught sooner in the processing?

As arrow stores all times as UTC I think it might be impossible 👍

[mhilton]

Is it the wrong hour? If I input a timestamp of 2024-10-27 02:30:00 Europe/Berlin

That is not the only way you could end up with such a timestamp though. The more likely thing is to have a stream of input from a machine produced in UTC. If one wants to display the output in a local timezone then one could use a query like:

SELECT date_trunc('hour', arrow_cast(time, 'Timestamp(Nanosecond, Some("UTC"))')) AS hour, COUNT(*) FROM events GROUP BY hour

if the earliest is always chosen for the ambiguous cases this would produce results like:

hour                        count(*)
2024-10-27 00:00:00 +0200   10
2024-10-27 01:00:00 +0200   10
2024-10-27 02:00:00 +0200   20
2024-10-27 02:00:00 +0100    0
2024-10-27 03:00:00 +0100   10
2024-10-27 04:00:00 +0100   10

Incidentally arrow doesn't like that time string so you can't input it anyway Parser error: Error parsing timestamp from '2024-10-27 02:30:00 Europe/Berlin': error computing timezone offset.

[tustvold]

Incidentally arrow doesn't like that time string so you can't input it anyway Parser error: Error parsing timestamp from '2024-10-27 02:30:00 Europe/Berlin': error computing timezone offset.

You have to enable the chrono-tz feature to parse non-offset timezones, it definitely supports them

The more likely thing is to have a stream of input from a machine produced in UTC. If one wants to display the output in a local timezone then one could use a query like

if the earliest is always chosen for the ambiguous cases this would produce results like:

I think we should see what postgres does here, I'm not entirely sure what the correct behaviour here is, very little is intuitive or logical when it comes to DST

[mhilton]

You have to enable the chrono-tz feature to parse non-offset timezones, it definitely supports them

I should have been clearer, '2024-10-26 02:30:00 Europe/Berlin' parses just fine. I think arrow is complaining because the time is ambiguous rather than not understood.

[alamb]

The consensus seems to be to turn this from a panic into an error with a clear error message ideally with how they can fix / workaround the issue.

I will file a follow on ticket about potentially choosing one of the alternate behaviors described above where we can discuss it further

[mhilton]

I would like to push back on this plan slightly. My problem is that at the beginning of the operation there is enough information to unambiguously truncate the time. It is only the implementation detail of converting to a local time and then back again that causes any given timestamp value to become ambiguous.

However changing the panic to an error would be a welcome step.

[alamb]

Thanks @mhilton - so perhaps we should:

1. @appletreeisyellow will make a PR that changes the panic to a "not yet implemented" error and adds tests that covers the relevant cases
2. We continue trying to get consensus on this ticket for how to handle this case

[alamb]

@mhilton could you possibly provide some example inputs / expected outputs for your proposal in #8899 (comment)?

[mhilton]

@mhilton could you possibly provide some example inputs / expected outputs for your proposal in #8899 (comment)?

This is what postgresql does for times around the DST switchover.

mhilton=# SET TIMEZONE='Europe/Berlin';
SET
mhilton=# SELECT date_trunc('day', column1) AS day, date_trunc('hour', column1) AS hour, column1 as time  FROM (VALUES (TIMESTAMPTZ '2024-10-26 23:30:10Z'), (TIMESTAMPTZ '2024-10-27 00:30:10Z'), (TIMESTAMPTZ '2024-10-27 01:30:10Z'), (TIMESTAMPTZ '2024-10-27 02:30:10Z'));
          day           |          hour          |          time          
------------------------+------------------------+------------------------
 2024-10-27 00:00:00+02 | 2024-10-27 01:00:00+02 | 2024-10-27 01:30:10+02
 2024-10-27 00:00:00+02 | 2024-10-27 02:00:00+02 | 2024-10-27 02:30:10+02
 2024-10-27 00:00:00+02 | 2024-10-27 02:00:00+01 | 2024-10-27 02:30:10+01
 2024-10-27 00:00:00+02 | 2024-10-27 03:00:00+01 | 2024-10-27 03:30:10+01

Here I made the input UTC so that it is equivalent to how arrow stores timestamp values. As you can see the trucated hours that have the same wall-clock value (2nd and 3rd row) still maintain the offset value from the time that was truncated.