-
Notifications
You must be signed in to change notification settings - Fork 217
refactor!: share partition timestamp parsing #3341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,8 @@ | ||
| //! Arrow reader-timezone handling for `TIMESTAMP` partition values. | ||
| //! Engine-independent timezone handling for `TIMESTAMP` partition values. | ||
| //! | ||
| //! Kernel never infers this timezone from the host or the Delta table. A connector supplies its | ||
| //! reader/session timezone through [`MapToStructOptions`], and no option means UTC. Timestamp | ||
| //! strings may carry their own offset or named timezone. This preserves the Arrow evaluator's | ||
| //! partition-value compatibility; the embedded value takes precedence. | ||
| //! timezone through [`MapToStructOptions`], and no option means UTC. A protocol-formatted ISO 8601 | ||
| //! timestamp carries its own offset, which takes precedence. | ||
| //! | ||
| //! # Terminology | ||
| //! | ||
|
|
@@ -12,10 +11,10 @@ | |
|
|
||
| use std::str::FromStr; | ||
|
|
||
| use chrono::{FixedOffset, LocalResult, NaiveDateTime, Offset, TimeDelta, TimeZone, Utc}; | ||
| use chrono::{DateTime, FixedOffset, LocalResult, NaiveDateTime, Offset, TimeDelta, TimeZone}; | ||
| use chrono_tz::Tz; | ||
|
|
||
| use crate::arrow::compute::kernels::cast_utils::string_to_datetime; | ||
| #[cfg(feature = "arrow-expression")] | ||
| use crate::expressions::MapToStructOptions; | ||
| use crate::{DeltaResult, Error}; | ||
|
|
||
|
|
@@ -53,12 +52,13 @@ impl FromStr for TimestampTimezone { | |
| } | ||
|
|
||
| impl TimestampTimezone { | ||
| /// Resolves the reader timezone from map-to-struct options, defaulting to UTC. | ||
| /// Resolves the timezone from map-to-struct options, defaulting to UTC. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error when the configured value is neither a recognized IANA timezone nor a | ||
| /// fixed offset in `+HH:MM` or `-HH:MM` form. | ||
| #[cfg(feature = "arrow-expression")] | ||
| pub(crate) fn try_from_options(options: &MapToStructOptions) -> DeltaResult<Self> { | ||
| match options.timestamp_timezone() { | ||
| Some(value) => value.parse(), | ||
|
|
@@ -68,23 +68,15 @@ impl TimestampTimezone { | |
|
|
||
| /// Parses a partition timestamp into microseconds since the Unix epoch. | ||
| /// | ||
| /// Arrow handles the timestamp grammar. An offset or timezone carried by `raw` takes | ||
| /// precedence over the configured reader timezone. For an offset-less value in a named reader | ||
| /// timezone, local clock transitions follow reader timestamp-cast semantics: | ||
| /// A protocol-formatted ISO 8601 value carries its own offset. A space-separated value uses | ||
| /// the configured timezone. For an offset-less value in a named timezone, local clock | ||
| /// transitions follow timestamp-cast semantics: | ||
| /// | ||
| /// - If a backward clock change makes a local time occur twice, use the earlier instant. | ||
| /// - If a forward clock change skips a local time, use the offset from before the change. For | ||
| /// example, `02:30` in a one-hour spring-forward gap is interpreted as `03:30`. | ||
| /// | ||
| /// Arrow rejects both transition cases, so named timezones use a fallback only when Arrow | ||
| /// cannot resolve the local timestamp. | ||
| pub(crate) fn parse_timestamp(self, raw: &str) -> Option<i64> { | ||
| match self { | ||
| Self::Named(timezone) => parse_timestamp_in_named_timezone(raw, timezone), | ||
| Self::Fixed(timezone) => string_to_datetime(&timezone, raw) | ||
| .ok() | ||
| .map(|timestamp| timestamp.timestamp_micros()), | ||
| } | ||
| parse_timestamp(raw, self) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -109,23 +101,49 @@ fn parse_two_digits(value: &str) -> Option<i32> { | |
| let [tens, ones] = value.as_bytes() else { | ||
| return None; | ||
| }; | ||
| (tens.is_ascii_digit() && ones.is_ascii_digit()) | ||
| .then_some(i32::from(*tens - b'0') * 10 + i32::from(*ones - b'0')) | ||
| if !tens.is_ascii_digit() || !ones.is_ascii_digit() { | ||
| return None; | ||
| } | ||
| Some(i32::from(*tens - b'0') * 10 + i32::from(*ones - b'0')) | ||
| } | ||
|
|
||
| /// Parses with Arrow, resolving only named-zone clock transitions that Arrow rejects. | ||
| fn parse_timestamp_in_named_timezone(raw: &str, timezone: Tz) -> Option<i64> { | ||
| if let Ok(timestamp) = string_to_datetime(&timezone, raw) { | ||
| return Some(timestamp.timestamp_micros()); | ||
| fn parse_timestamp(raw: &str, timezone: TimestampTimezone) -> Option<i64> { | ||
| if raw | ||
| .split_once('.') | ||
| .is_some_and(|(_, suffix)| suffix.bytes().take_while(u8::is_ascii_digit).count() > 6) | ||
| { | ||
| return None; | ||
| } | ||
| let local_datetime = string_to_datetime(&Utc, raw).ok()?.naive_utc(); | ||
| match timezone.from_local_datetime(&local_datetime) { | ||
| LocalResult::Ambiguous(first, second) => Some(first.min(second).timestamp_micros()), | ||
| LocalResult::None => resolve_nonexistent_local_timestamp(local_datetime, timezone), | ||
| LocalResult::Single(_) => { | ||
| // A unique local time means Arrow rejected the input for another reason. | ||
| None | ||
| } | ||
| if let Ok(local_datetime) = NaiveDateTime::parse_from_str(raw, "%Y-%m-%d %H:%M:%S%.f") { | ||
| return resolve_local_timestamp(local_datetime, timezone); | ||
| } | ||
| parse_explicit_offset_timestamp(raw) | ||
| } | ||
|
|
||
| fn parse_explicit_offset_timestamp(raw: &str) -> Option<i64> { | ||
|
DrakeLin marked this conversation as resolved.
|
||
| // RFC 3339 permits lowercase separators, but the protocol form uses uppercase `T` and `Z`. | ||
| if raw.as_bytes().get(10) != Some(&b'T') || raw.ends_with('z') { | ||
| return None; | ||
| } | ||
| let timestamp = DateTime::parse_from_rfc3339(raw).ok()?; | ||
| (timestamp.offset().local_minus_utc().unsigned_abs() <= 18 * 3_600) | ||
| .then(|| timestamp.timestamp_micros()) | ||
| } | ||
|
|
||
| fn resolve_local_timestamp( | ||
| local_datetime: NaiveDateTime, | ||
| timezone: TimestampTimezone, | ||
| ) -> Option<i64> { | ||
| match timezone { | ||
| TimestampTimezone::Fixed(timezone) => timezone | ||
| .from_local_datetime(&local_datetime) | ||
| .single() | ||
| .map(|timestamp| timestamp.timestamp_micros()), | ||
| TimestampTimezone::Named(timezone) => match timezone.from_local_datetime(&local_datetime) { | ||
| LocalResult::Ambiguous(first, second) => Some(first.min(second).timestamp_micros()), | ||
| LocalResult::None => resolve_nonexistent_local_timestamp(local_datetime, timezone), | ||
| LocalResult::Single(timestamp) => Some(timestamp.timestamp_micros()), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -148,25 +166,27 @@ fn resolve_nonexistent_local_timestamp(local_datetime: NaiveDateTime, timezone: | |
| mod tests { | ||
| use rstest::rstest; | ||
|
|
||
| use super::super::expected_timestamp_micros; | ||
| use super::*; | ||
|
|
||
| fn options(timezone: &str) -> MapToStructOptions { | ||
| MapToStructOptions::default().with_timestamp_timezone(timezone) | ||
| fn expected_timestamp_micros(timestamp: &str) -> i64 { | ||
| DateTime::parse_from_rfc3339(timestamp) | ||
| .unwrap() | ||
| .timestamp_micros() | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case::named("America/Los_Angeles")] | ||
| #[case::minute_offset("+05:30")] | ||
| #[case::positive_limit("+18:00")] | ||
| #[case::negative_limit("-18:00")] | ||
| fn accepts_normalized_reader_timezones(#[case] timezone: &str) { | ||
| assert!(TimestampTimezone::try_from_options(&options(timezone)).is_ok()); | ||
| fn accepts_normalized_timezones(#[case] timezone: &str) { | ||
| assert!(timezone.parse::<TimestampTimezone>().is_ok()); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case::empty("")] | ||
| #[case::bare_sign("+")] | ||
| #[case::malformed_sign("+-5:00")] | ||
| #[case::compact_hour("+05")] | ||
| #[case::compact_hour_minute("+0530")] | ||
| #[case::invalid_minutes("+05:60")] | ||
|
|
@@ -176,37 +196,38 @@ mod tests { | |
| #[case::invalid_hours("+19:00")] | ||
| #[case::extra_component("+05:00:00:00")] | ||
| #[case::unknown_name("Not/AZone")] | ||
| fn rejects_noncanonical_reader_timezones(#[case] timezone: &str) { | ||
| assert!(TimestampTimezone::try_from_options(&options(timezone)).is_err()); | ||
| fn rejects_noncanonical_timezones(#[case] timezone: &str) { | ||
| assert!(timezone.parse::<TimestampTimezone>().is_err()); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case::local("2024-01-15 12:30:45.123456", "2024-01-15T12:30:45.123456Z")] | ||
| #[case::compact_offset("2024-01-15T17:30:45+0530", "2024-01-15T12:00:45Z")] | ||
| #[case::offset_beyond_reader_limit("2024-01-15T12:30:45+19:00", "2024-01-14T17:30:45Z")] | ||
| #[case::utc("2024-01-15T12:30:45.123456Z", "2024-01-15T12:30:45.123456Z")] | ||
| #[case::positive_offset("2024-01-15T17:30:45+05:30", "2024-01-15T12:00:45Z")] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit2 The 18h embedded-offset bound ( Raised by: test-coverage-reviewer. Suggested fix: add an accept case at the boundary, e.g. |
||
| #[case::negative_offset("2024-01-15T07:00:45-05:30", "2024-01-15T12:30:45Z")] | ||
| #[case::spaced_offset("2024-01-15 17:30:45 +05:30", "2024-01-15T12:00:45Z")] | ||
| #[case::named("2024-01-15 12:30:45 America/New_York", "2024-01-15T17:30:45Z")] | ||
| #[case::lowercase_t("2024-01-15t12:30:45", "2024-01-15T12:30:45Z")] | ||
| #[case::compact_clock("2024-01-15 123045", "2024-01-15T12:30:45Z")] | ||
| #[case::date_only("2024-01-15", "2024-01-15T00:00:00Z")] | ||
| #[case::excess_fraction("2024-01-15 12:30:45.123456789123", "2024-01-15T12:30:45.123456Z")] | ||
| fn parses_compatible_partition_timestamps(#[case] raw: &str, #[case] expected: &str) { | ||
| fn parses_protocol_partition_timestamps(#[case] raw: &str, #[case] expected: &str) { | ||
| assert_eq!( | ||
| TimestampTimezone::default().parse_timestamp(raw), | ||
| Some(expected_timestamp_micros(expected)) | ||
| ); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case::conflicting("2024-01-15 12:30:45+02:00 America/New_York")] | ||
| #[case::unknown_embedded("2024-01-15 12:30:45 Foo/Bar")] | ||
| #[case::multibyte_prefix("日時 America/New_York")] | ||
| #[case::embedded_timezone("2024-01-15 12:30:45 America/New_York")] | ||
| #[case::spaced_offset("2024-01-15 17:30:45 +05:30")] | ||
| #[case::lowercase_t("2024-01-15t12:30:45")] | ||
| #[case::compact_clock("2024-01-15 123045")] | ||
| #[case::date_only("2024-01-15")] | ||
| #[case::zoneless_t("2024-01-15T12:30:45")] | ||
| #[case::compact_offset("2024-01-15T12:30:45+0530")] | ||
| #[case::hour_only_offset("2024-01-15T12:30:45+07")] | ||
| #[case::offset_past_limit("2024-01-15T12:30:45+19:00")] | ||
| #[case::sub_microsecond_precision("1969-12-31 23:59:59.999999500")] | ||
| #[case::ten_digit_fraction("2024-01-15 12:30:45.1234567891")] | ||
| #[case::extra_offset_component("2024-01-15T12:30:45+05:30:15:00")] | ||
| #[case::bad_compact_offset("2024-01-15T12:30:45+053")] | ||
| #[case::ampm("2024-01-15 01:30:45 PM")] | ||
| #[case::day_first("15/01/2024 12:30:45")] | ||
| #[case::time_only("12:30:45")] | ||
| #[case::invalid_suffix("2024-01-15T12:30:45XYZ")] | ||
| #[case::trailing_garbage_after_zone("2024-01-15T12:30:45ZXYZ")] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit1 The Raised by: test-coverage-reviewer. Suggested fix: add a reject case exercising the guard, e.g. |
||
| fn rejects_unsupported_partition_timestamps(#[case] raw: &str) { | ||
| assert_eq!(TimestampTimezone::default().parse_timestamp(raw), None); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.