fix: handle ambiguous and non-existent local times

[matthewalex4]

Which issue does this PR close?

Closes #3864.

Rationale for this change

timestamp_ntz_to_timestamp panics when a local time is ambiguous or non-existent due to DST transitions. Further explanation in issue description.

What changes are included in this PR?

• The result of from_local_datetime(...) is handled for cases of ambiguous and non-existent local times.
• For ambiguous local times we choose the earlier option
• For non-existent local times we shift to an hour before, convert and shift back

How are these changes tested?

Unit tests provided for both ambiguous and non-existent local times.

[andygrove]

The 1-hour shift works for standard DST transitions, but some timezones have non-standard gaps (e.g., Australia/Lord_Howe has a 30-minute DST transition). Yes, this is an edge case so it may be fine to ignore, but we should at least document this as an incompatibility.

https://www.timeanddate.com/time/change/australia/lord-howe-island

[matthewalex4]

Added a comment in d622e44

[parthchandra]

I'm handling the DST gap in #3884. Perhaps you can wait for this to pass CI and then re-use.

[parthchandra]

Consider doing something like in string.rs parse_timestamp_to_micros. Something like -

  LocalResult::None => {
      let probe = local_datetime - chrono::Duration::hours(3);
      let pre_offset = match tz.from_local_datetime(&probe) {
          LocalResult::Single(dt) => dt.offset().fix(),
          LocalResult::Ambiguous(dt, _) => dt.offset().fix(),
          // Cannot determine offset – return some safe fallback or propagate error
          LocalResult::None => {
              // Extreme edge case; fall back to UTC interpretation
              local_datetime.and_utc().with_timezone(tz)
          }
      };
      let offset_secs = pre_offset.local_minus_utc() as i64;
      let utc_naive = local_datetime - chrono::Duration::seconds(offset_secs);
      utc_naive.and_utc().with_timezone(tz)
  }

This eliminates the unwrap(), handles non-standard DST correctly, and reuses a pattern that already has test coverage in Comet.

[andygrove]

do these tests still need catch_unwind?

[matthewalex4]

No not necessary, removed in d622e44

[andygrove]

Thanks @matthewalex4. Could you also add an end-to-end Spark test, perhaps in CometTemporalExpressionSuite?

[matthewalex4]

Thanks @matthewalex4. Could you also add an end-to-end Spark test, perhaps in CometTemporalExpressionSuite?

Yes, have added a test in 998f975

[parthchandra]

Consider doing something like in string.rs parse_timestamp_to_micros. Something like -

  LocalResult::None => {
      let probe = local_datetime - chrono::Duration::hours(3);
      let pre_offset = match tz.from_local_datetime(&probe) {
          LocalResult::Single(dt) => dt.offset().fix(),
          LocalResult::Ambiguous(dt, _) => dt.offset().fix(),
          // Cannot determine offset – return some safe fallback or propagate error
          LocalResult::None => {
              // Extreme edge case; fall back to UTC interpretation
              local_datetime.and_utc().with_timezone(tz)
          }
      };
      let offset_secs = pre_offset.local_minus_utc() as i64;
      let utc_naive = local_datetime - chrono::Duration::seconds(offset_secs);
      utc_naive.and_utc().with_timezone(tz)
  }

This eliminates the unwrap(), handles non-standard DST correctly, and reuses a pattern that already has test coverage in Comet.

[parthchandra]

Merged. Thank you @matthewalex4