andygrove commented on code in PR #5130:
URL: https://github.com/apache/datafusion-comet/pull/5130#discussion_r3969670690
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -2941,6 +3195,532 @@ mod tests {
);
}
+ /// Days from 1970-01-01 for a proleptic-Gregorian civil date (Howard
Hinnant's algorithm).
+ ///
+ /// Used to compute the expected values of the ported Spark tests below
independently of
+ /// the parser under test. Handles the full year range Spark supports,
including negative
+ /// years and years beyond `chrono`'s limits, which the
`Long.MinValue`/`MaxValue`
+ /// boundary cases need.
+ fn days_from_civil(year: i64, month: i64, day: i64) -> i64 {
+ let y = year - i64::from(month <= 2);
+ // Truncating division, as the algorithm requires - not Rust's
`%`-consistent one.
+ let era = if y >= 0 { y } else { y - 399 } / 400;
+ let year_of_era = y - era * 400; // [0, 399]
+ let day_of_year = (153 * (month + if month > 2 { -3 } else { 9 }) + 2)
/ 5 + day - 1;
+ let day_of_era = year_of_era * 365 + year_of_era / 4 - year_of_era /
100 + day_of_year;
+ era * 146097 + day_of_era - 719468
+ }
+
+ /// Microseconds since the epoch for a UTC civil date-time. The Rust
equivalent of the
+ /// `DateTimeTestUtils.date(..., zid = UTC)` helper the Spark tests below
use.
+ ///
+ /// Computed in `i128` because the `Long.MinValue` boundary case below is
exactly
+ /// `i64::MIN` microseconds, and its intermediate whole-second product
overflows `i64`.
+ fn utc_micros(
+ year: i64,
+ month: i64,
+ day: i64,
+ hour: i64,
+ minute: i64,
+ second: i64,
+ micros: i64,
+ ) -> i64 {
+ let seconds = i128::from(days_from_civil(year, month, day)) * 86_400
+ + i128::from(hour) * 3_600
+ + i128::from(minute) * 60
+ + i128::from(second);
+ i64::try_from(seconds * 1_000_000 + i128::from(micros))
+ .expect("expected value should fit in i64")
+ }
+
+ /// Reinterprets a UTC-computed instant as a wall-clock time in a fixed
zone
+ /// `offset_seconds` east of UTC. The equivalent of `date(..., zid =
getZoneId("+07:30"))`.
+ fn at_offset(utc_micros: i64, offset_seconds: i64) -> i64 {
+ utc_micros - offset_seconds * 1_000_000
+ }
+
+ /// The inputs for which Comet's ANSI mode returns null where Spark raises
+ /// CAST_INVALID_INPUT: a leading `+` that is not followed by a
`<digits>-` year.
+ ///
+ /// This predicate exists only to keep the ported Spark tests below honest
about the
+ /// divergence rather than skipping the ANSI dimension for them entirely.
Tracked by
+ /// <https://github.com/apache/datafusion-comet/issues/5165>.
+ fn ansi_returns_null_instead_of_raising(value: &str) -> bool {
+ let trimmed = value.trim();
+ match trimmed.strip_prefix('+') {
+ None => false,
+ Some(rest) => !matches!(
+ rest.find(|c: char| !c.is_ascii_digit()),
+ Some(i) if i >= 1 && rest.as_bytes()[i] == b'-'
+ ),
+ }
+ }
+
+ /// Asserts `value` parses to `expected` in every eval mode, under session
timezone `tz`.
+ ///
+ /// Mirrors `checkStringToTimestamp` in Spark's `DateTimeUtilsSuite`,
extended over the
+ /// eval modes Comet has: a `None` expectation additionally requires ANSI
mode to raise
+ /// rather than return null.
+ fn check_string_to_timestamp(value: &str, expected: Option<i64>, tz: &str)
{
+ let tz = &Tz::from_str(tz).unwrap();
+ for is_spark4_plus in [false, true] {
+ for eval_mode in [EvalMode::Legacy, EvalMode::Try] {
+ assert_eq!(
+ timestamp_parser(value, eval_mode, tz,
is_spark4_plus).unwrap(),
+ expected,
+ "{value:?} in {eval_mode:?} (spark4={is_spark4_plus})"
+ );
+ }
+ let ansi = timestamp_parser(value, EvalMode::Ansi, tz,
is_spark4_plus);
+ match expected {
+ Some(_) => assert_eq!(
+ ansi.unwrap(),
+ expected,
+ "{value:?} in ANSI (spark4={is_spark4_plus})"
+ ),
+ // Spark raises CAST_INVALID_INPUT under ANSI for anything it
cannot parse,
+ // except that an all-whitespace or empty string is null in
every mode.
+ None if value.trim().is_empty() => {
+ assert_eq!(ansi.unwrap(), None, "{value:?} in ANSI")
+ }
+ // Pre-existing gap, not introduced by the shape classifier: a
leading '+'
+ // that is not a year sign returns null in every mode, where
Spark raises
+ // under ANSI. Tracked by issue 5165; asserted here so it
cannot widen.
+ None if ansi_returns_null_instead_of_raising(value) => {
+ assert_eq!(ansi.unwrap(), None, "{value:?} in ANSI")
+ }
+ None => assert!(
+ ansi.is_err(),
+ "{value:?} should raise in ANSI (spark4={is_spark4_plus})"
+ ),
+ }
+ }
+ }
+
+ /// Port of `test("string to timestamp")` in Spark's `DateTimeUtilsSuite`.
+ ///
+ /// Spark runs its version over `ALL_TIMEZONES`; the session-timezone
dimension is covered
+ /// separately by `spark_string_to_timestamp_session_timezone_test` below,
so this pins the
+ /// exact expected microseconds under a UTC session timezone.
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn spark_string_to_timestamp_test() {
+ let check =
+ |value: &str, expected: Option<i64>|
check_string_to_timestamp(value, expected, "UTC");
+
+ check(
+ "1969-12-31 16:00:00",
+ Some(utc_micros(1969, 12, 31, 16, 0, 0, 0)),
+ );
+ check("0001", Some(utc_micros(1, 1, 1, 0, 0, 0, 0)));
+ check("2015-03", Some(utc_micros(2015, 3, 1, 0, 0, 0, 0)));
+ for value in ["2015-03-18", "2015-03-18 ", " 2015-03-18", " 2015-03-18
"] {
+ check(value, Some(utc_micros(2015, 3, 18, 0, 0, 0, 0)));
+ }
+
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 0));
+ check("2015-03-18 12:03:17", expected);
+ check("2015-03-18T12:03:17", expected);
+
+ // When the string carries a timezone, that zone wins over the session
timezone.
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 0),
+ -(13 * 3600 + 53 * 60),
+ ));
+ check("2015-03-18T12:03:17-13:53", expected);
+ check("2015-03-18T12:03:17GMT-13:53", expected);
+ check("2015-03-18T12:03:17-1353", expected);
+
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 0));
+ check("2015-03-18T12:03:17Z", expected);
+ check("2015-03-18 12:03:17Z", expected);
+ check("2015-03-18 12:03:17UTC", expected);
+
+ let expected = Some(at_offset(utc_micros(2015, 3, 18, 12, 3, 17, 0),
-3600));
+ check("2015-03-18T12:03:17-1:0", expected);
+ check("2015-03-18T12:03:17-01:00", expected);
+ check("2015-03-18T12:03:17GMT-01:00", expected);
+ check("2015-03-18T12:03:17-0100", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 0),
+ 7 * 3600 + 30 * 60,
+ ));
+ check("2015-03-18T12:03:17+07:30", expected);
+ check("2015-03-18T12:03:17 GMT+07:30", expected);
+ check("2015-03-18T12:03:17+0730", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 0),
+ 7 * 3600 + 3 * 60,
+ ));
+ check("2015-03-18T12:03:17+07:03", expected);
+ check("2015-03-18T12:03:17GMT+07:03", expected);
+ check("2015-03-18T12:03:17+0703", expected);
+
+ // Strings including milliseconds.
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 123_000));
+ check("2015-03-18 12:03:17.123", expected);
+ check("2015-03-18T12:03:17.123", expected);
+
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 456_000));
+ check("2015-03-18T12:03:17.456Z", expected);
+ check("2015-03-18 12:03:17.456Z", expected);
+ check("2015-03-18 12:03:17.456 UTC", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_000),
+ -3600,
+ ));
+ check("2015-03-18T12:03:17.123-1:0", expected);
+ check("2015-03-18T12:03:17.123-01:00", expected);
+ check("2015-03-18T12:03:17.123 GMT-01:00", expected);
+ check("2015-03-18T12:03:17.123-0100", expected);
+
+ let plus_730 = 7 * 3600 + 30 * 60;
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_000),
+ plus_730,
+ ));
+ check("2015-03-18T12:03:17.123+07:30", expected);
+ check("2015-03-18T12:03:17.123 GMT+07:30", expected);
+ check("2015-03-18T12:03:17.123+0730", expected);
+ check("2015-03-18T12:03:17.123GMT+07:30", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_121),
+ plus_730,
+ ));
+ check("2015-03-18T12:03:17.123121+7:30", expected);
+ check("2015-03-18T12:03:17.123121 GMT+0730", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_120),
+ plus_730,
+ ));
+ check("2015-03-18T12:03:17.12312+7:30", expected);
+ check("2015-03-18T12:03:17.12312 UT+07:30", expected);
+ check("2015-03-18T12:03:17.12312+0730", expected);
+
+ // Time-only strings are anchored to the current date, so assert
against a
+ // today-relative expectation rather than a fixed instant.
+ let today = chrono::Utc::now().date_naive();
Review Comment:
Fixed in ad2e83017. You're right, and the anchor is the string's own zone
rather than UTC — the parser resolves it with
`tz.from_utc_datetime(Utc::now())`, and for a string carrying its own offset
that `tz` **is** the offset from the string. The expectation was built from
`chrono::Utc::now().date_naive()`, so it disagreed for exactly the part of the
day you identified.
`today_micros` now derives the date from the offset it is given:
```rust
let today = (chrono::Utc::now() +
chrono::Duration::seconds(offset)).date_naive();
```
One thing I wanted to avoid was fixing this and leaving a test that only
proves it for part of the day — at 14:35 UTC the `+07:30` cases pass either
way, so a green run says nothing. I added two permanent cases with extreme
offsets in opposite directions:
```rust
check("T18:12:15+14:00", today_micros(18, 12, 15, 0, 14 * 3600));
check("T18:12:15-12:00", today_micros(18, 12, 15, 0, -12 * 3600));
```
`+14:00` crosses the date line from 10:00 UTC onward and `-12:00` crosses
before 12:00 UTC, so between them one is always in the failing window whatever
hour CI runs at. Checked against the old code at 14:35 UTC and the `+14:00`
case fails by exactly 86,400 s:
```
left: Some(1789013535000000)
right: Some(1788927135000000)
```
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1619,87 +1574,311 @@ fn extract_offset_suffix(value: &str) -> Option<(&str,
timezone::Tz)> {
None
}
-type TimestampParsePattern<T> = (&'static Regex, fn(&str, &T) ->
SparkResult<Option<i64>>);
-
-// RE_YEAR allows only 4-6 digits (not 7) because a bare 7-digit string like
"0119704"
-// is ambiguous and Spark rejects it. The other patterns (RE_MONTH, RE_DAY,
etc.) keep
-// \d{4,7} because the `-` separator disambiguates the year portion, so
"0002020-01-01"
-// is validly year 2020 with leading zeros. date_parser's is_valid_digits also
allows up
-// to 7 year digits for the same reason.
-static RE_YEAR: LazyLock<Regex> = LazyLock::new(||
Regex::new(r"^-?\d{4,6}$").unwrap());
-static RE_MONTH: LazyLock<Regex> = LazyLock::new(||
Regex::new(r"^-?\d{4,7}-\d{2}$").unwrap());
-static RE_DAY: LazyLock<Regex> = LazyLock::new(||
Regex::new(r"^-?\d{4,7}-\d{2}-\d{2}$").unwrap());
-static RE_HOUR: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^-?\d{4,7}-\d{2}-\d{2}[T
]\d{1,2}$").unwrap());
-static RE_MINUTE: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^-?\d{4,7}-\d{2}-\d{2}[T
]\d{2}:\d{2}$").unwrap());
-static RE_SECOND: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^-?\d{4,7}-\d{2}-\d{2}[T
]\d{2}:\d{2}:\d{2}$").unwrap());
-static RE_MICROSECOND: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^-?\d{4,7}-\d{2}-\d{2}[T
]\d{2}:\d{2}:\d{2}\.\d+$").unwrap());
-static RE_TIME_ONLY_H: LazyLock<Regex> = LazyLock::new(||
Regex::new(r"^T\d{1,2}$").unwrap());
-static RE_TIME_ONLY_HM: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^T\d{1,2}:\d{1,2}$").unwrap());
-static RE_TIME_ONLY_HMS: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^T\d{1,2}:\d{1,2}:\d{1,2}$").unwrap());
-static RE_TIME_ONLY_HMSU: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^T\d{1,2}:\d{1,2}:\d{1,2}\.\d+$").unwrap());
-static RE_BARE_HM: LazyLock<Regex> = LazyLock::new(||
Regex::new(r"^\d{1,2}:\d{1,2}$").unwrap());
-static RE_BARE_HMS: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^\d{1,2}:\d{1,2}:\d{1,2}$").unwrap());
-static RE_BARE_HMSU: LazyLock<Regex> =
- LazyLock::new(|| Regex::new(r"^\d{1,2}:\d{1,2}:\d{1,2}\.\d+$").unwrap());
+/// The timestamp string shapes the parser recognises, listed in the order
they are matched.
+/// The shapes are mutually exclusive, so at most one can apply to any given
string.
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+enum TimestampPattern {
+ Year,
+ Month,
+ Day,
+ Hour,
+ Minute,
+ Second,
+ Microsecond,
+ TimeOnlyH,
+ TimeOnlyHm,
+ TimeOnlyHms,
+ TimeOnlyHmsu,
+ BareHm,
+ BareHms,
+ BareHmsu,
+}
+
+impl TimestampPattern {
+ /// Every shape, in the order they are matched. First match wins, so the
order matters.
+ const ALL: [TimestampPattern; 14] = [
+ Self::Year,
+ Self::Month,
+ Self::Day,
+ Self::Hour,
+ Self::Minute,
+ Self::Second,
+ Self::Microsecond,
+ Self::TimeOnlyH,
+ Self::TimeOnlyHm,
+ Self::TimeOnlyHms,
+ Self::TimeOnlyHmsu,
+ Self::BareHm,
+ Self::BareHms,
+ Self::BareHmsu,
+ ];
+
+ /// The equivalent regular expression for this shape.
+ ///
+ /// Only used for the rare non-ASCII input, where the Unicode-aware `\d`
class accepts
+ /// digits (e.g. Arabic-Indic) that the ASCII classifier below does not.
+ ///
+ /// `Year` allows only 4-6 digits (not 7) because a bare 7-digit string
like "0119704" is
+ /// ambiguous and Spark rejects it. The others keep `\d{4,7}` because the
`-` separator
+ /// disambiguates the year portion, so "0002020-01-01" is validly year
2020 with leading
+ /// zeros. `date_parser`'s `is_valid_digits` allows up to 7 year digits
for the same reason.
+ fn regex_str(self) -> &'static str {
+ match self {
+ Self::Year => r"^-?\d{4,6}$",
+ Self::Month => r"^-?\d{4,7}-\d{2}$",
+ Self::Day => r"^-?\d{4,7}-\d{2}-\d{2}$",
+ Self::Hour => r"^-?\d{4,7}-\d{2}-\d{2}[T ]\d{1,2}$",
+ Self::Minute => r"^-?\d{4,7}-\d{2}-\d{2}[T ]\d{2}:\d{2}$",
+ Self::Second => r"^-?\d{4,7}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}$",
+ Self::Microsecond => r"^-?\d{4,7}-\d{2}-\d{2}[T
]\d{2}:\d{2}:\d{2}\.\d+$",
+ Self::TimeOnlyH => r"^T\d{1,2}$",
+ Self::TimeOnlyHm => r"^T\d{1,2}:\d{1,2}$",
+ Self::TimeOnlyHms => r"^T\d{1,2}:\d{1,2}:\d{1,2}$",
+ Self::TimeOnlyHmsu => r"^T\d{1,2}:\d{1,2}:\d{1,2}\.\d+$",
+ Self::BareHm => r"^\d{1,2}:\d{1,2}$",
+ Self::BareHms => r"^\d{1,2}:\d{1,2}:\d{1,2}$",
+ Self::BareHmsu => r"^\d{1,2}:\d{1,2}:\d{1,2}\.\d+$",
+ }
+ }
+
+ /// True for the shapes that carry no date component: `T12`, `T12:34`,
`12:34`, ...
+ fn is_time_only(self) -> bool {
+ !matches!(
+ self,
+ Self::Year
+ | Self::Month
+ | Self::Day
+ | Self::Hour
+ | Self::Minute
+ | Self::Second
+ | Self::Microsecond
+ )
+ }
+
+ /// True for the `T`-prefixed time-only shapes only, which Spark 4.0+
rejects when the
+ /// raw value has leading whitespace.
+ fn is_t_time_only(self) -> bool {
+ matches!(
+ self,
+ Self::TimeOnlyH | Self::TimeOnlyHm | Self::TimeOnlyHms |
Self::TimeOnlyHmsu
+ )
+ }
+}
+
+static TIMESTAMP_PATTERN_SET: LazyLock<RegexSet> = LazyLock::new(|| {
+
RegexSet::new(TimestampPattern::ALL.map(TimestampPattern::regex_str)).unwrap()
+});
+
+/// Returns the shape `value` has, or `None` when it matches none of them.
+///
+/// ASCII input - effectively all real data - is classified by a single
left-to-right byte
+/// scan. Non-ASCII input falls back to a single `RegexSet` pass, which
reports every
+/// matching pattern in one search of the haystack; the lowest matching index
is taken so
+/// that the result is the same first-match-wins answer the ASCII scan gives.
+fn classify_timestamp_pattern(value: &str) -> Option<TimestampPattern> {
+ if value.is_ascii() {
+ classify_ascii_timestamp_pattern(value.as_bytes())
+ } else {
+ TIMESTAMP_PATTERN_SET
+ .matches(value)
+ .iter()
+ .next()
+ .map(|i| TimestampPattern::ALL[i])
+ }
+}
+
+/// Number of leading ASCII digits in `bytes`.
+fn digit_run(bytes: &[u8]) -> usize {
+ bytes
+ .iter()
+ .position(|b| !b.is_ascii_digit())
+ .unwrap_or(bytes.len())
+}
+
+/// True when `bytes` is a non-empty run of ASCII digits, i.e. a `\.\d+`
fraction tail.
+fn all_digits(bytes: &[u8]) -> bool {
+ !bytes.is_empty() && digit_run(bytes) == bytes.len()
+}
+
+/// ASCII-only equivalent of matching `value` against each
[`TimestampPattern::regex_str`] in
+/// order.
+fn classify_ascii_timestamp_pattern(bytes: &[u8]) -> Option<TimestampPattern> {
+ // `T`-prefixed time-only shapes.
+ if let [b'T', rest @ ..] = bytes {
+ return classify_ascii_time(rest, true);
+ }
+
+ let (negative, rest) = match bytes.split_first() {
+ Some((b'-', rest)) => (true, rest),
+ Some(_) => (false, bytes),
+ None => return None,
+ };
+
+ let digits = digit_run(rest);
+ let after_digits = &rest[digits..];
+ if after_digits.is_empty() {
+ // Year only.
+ return (4..=6).contains(&digits).then_some(TimestampPattern::Year);
+ }
+ match after_digits[0] {
+ // A year of 4-7 digits followed by the date separator.
+ b'-' if (4..=7).contains(&digits) =>
classify_ascii_date_tail(&after_digits[1..]),
+ // Bare time-only shapes take a 1-2 digit hour and no sign.
+ b':' if !negative && (1..=2).contains(&digits) =>
classify_ascii_time(rest, false),
+ _ => None,
+ }
+}
+
+/// Classifies `\d{1,2}(:\d{1,2}(:\d{1,2}(\.\d+)?)?)?`, the time-only shapes.
A bare hour
+/// with no minutes is only a shape when the `T` prefix was present.
+fn classify_ascii_time(bytes: &[u8], t_prefixed: bool) ->
Option<TimestampPattern> {
+ let hour = digit_run(bytes);
+ if !(1..=2).contains(&hour) {
+ return None;
+ }
+ let bytes = &bytes[hour..];
+ if bytes.is_empty() {
+ return t_prefixed.then_some(TimestampPattern::TimeOnlyH);
+ }
+ let [b':', bytes @ ..] = bytes else {
+ return None;
+ };
+
+ let minute = digit_run(bytes);
+ if !(1..=2).contains(&minute) {
+ return None;
+ }
+ let bytes = &bytes[minute..];
+ if bytes.is_empty() {
+ return Some(if t_prefixed {
+ TimestampPattern::TimeOnlyHm
+ } else {
+ TimestampPattern::BareHm
+ });
+ }
+ let [b':', bytes @ ..] = bytes else {
+ return None;
+ };
+
+ let second = digit_run(bytes);
+ if !(1..=2).contains(&second) {
+ return None;
+ }
+ let bytes = &bytes[second..];
+ if bytes.is_empty() {
+ return Some(if t_prefixed {
+ TimestampPattern::TimeOnlyHms
+ } else {
+ TimestampPattern::BareHms
+ });
+ }
+ let [b'.', fraction @ ..] = bytes else {
+ return None;
+ };
+ all_digits(fraction).then_some(if t_prefixed {
+ TimestampPattern::TimeOnlyHmsu
+ } else {
+ TimestampPattern::BareHmsu
+ })
+}
+
+/// Classifies `\d{2}(-\d{2}([T ]\d{1,2}(:\d{2}(:\d{2}(\.\d+)?)?)?)?)?`, the
part of a
+/// date-time shape that follows the year and its `-` separator. Note the
asymmetry the
+/// patterns encode: the hour is 1-2 digits when it ends the string, but
exactly 2 digits
+/// once a minute follows.
+fn classify_ascii_date_tail(bytes: &[u8]) -> Option<TimestampPattern> {
+ if digit_run(bytes) != 2 {
+ return None;
+ }
+ let bytes = &bytes[2..];
+ if bytes.is_empty() {
+ return Some(TimestampPattern::Month);
+ }
+ let [b'-', bytes @ ..] = bytes else {
+ return None;
+ };
+
+ if digit_run(bytes) != 2 {
+ return None;
+ }
+ let bytes = &bytes[2..];
+ if bytes.is_empty() {
+ return Some(TimestampPattern::Day);
+ }
+ let ([b'T', bytes @ ..] | [b' ', bytes @ ..]) = bytes else {
+ return None;
+ };
+
+ let hour = digit_run(bytes);
+ if !(1..=2).contains(&hour) {
+ return None;
+ }
+ let after_hour = &bytes[hour..];
+ if after_hour.is_empty() {
+ return Some(TimestampPattern::Hour);
+ }
+ // Everything past the hour requires a two-digit hour followed by a colon.
+ if hour != 2 {
+ return None;
+ }
+ let [b':', bytes @ ..] = after_hour else {
+ return None;
+ };
+
+ if digit_run(bytes) != 2 {
+ return None;
+ }
+ let bytes = &bytes[2..];
+ if bytes.is_empty() {
+ return Some(TimestampPattern::Minute);
+ }
+ let [b':', bytes @ ..] = bytes else {
+ return None;
+ };
+
+ if digit_run(bytes) != 2 {
+ return None;
+ }
+ let bytes = &bytes[2..];
+ if bytes.is_empty() {
+ return Some(TimestampPattern::Second);
+ }
+ let [b'.', fraction @ ..] = bytes else {
+ return None;
+ };
+ all_digits(fraction).then_some(TimestampPattern::Microsecond)
+}
fn timestamp_parser_with_tz<T: TimeZone>(
value: &str,
eval_mode: EvalMode,
tz: &T,
) -> SparkResult<Option<i64>> {
- // Both T-separator and space-separator date-time forms are supported.
- // Negative years are handled by get_timestamp_values detecting a leading
'-'.
- let patterns: &[TimestampParsePattern<T>] = &[
- // Year only: 4-7 digits, optionally negative
- (
- &RE_YEAR,
- parse_str_to_year_timestamp as fn(&str, &T) ->
SparkResult<Option<i64>>,
- ),
- // Year-month
- (&RE_MONTH, parse_str_to_month_timestamp),
- // Year-month-day
- (&RE_DAY, parse_str_to_day_timestamp),
- // Date T-or-space hour (1 or 2 digits)
- (&RE_HOUR, parse_str_to_hour_timestamp),
- // Date T-or-space hour:minute
- (&RE_MINUTE, parse_str_to_minute_timestamp),
- // Date T-or-space hour:minute:second
- (&RE_SECOND, parse_str_to_second_timestamp),
- // Date T-or-space hour:minute:second.fraction
- (&RE_MICROSECOND, parse_str_to_microsecond_timestamp),
- // Time-only: T hour (1 or 2 digits, no colon)
- (&RE_TIME_ONLY_H, parse_str_to_time_only_timestamp),
- // Time-only: T hour:minute
- (&RE_TIME_ONLY_HM, parse_str_to_time_only_timestamp),
- // Time-only: T hour:minute:second
- (&RE_TIME_ONLY_HMS, parse_str_to_time_only_timestamp),
- // Time-only: T hour:minute:second.fraction
- (&RE_TIME_ONLY_HMSU, parse_str_to_time_only_timestamp),
- // Bare time-only: hour:minute (without T prefix)
- (&RE_BARE_HM, parse_str_to_time_only_timestamp),
- // Bare time-only: hour:minute:second
- (&RE_BARE_HMS, parse_str_to_time_only_timestamp),
- // Bare time-only: hour:minute:second.fraction
- (&RE_BARE_HMSU, parse_str_to_time_only_timestamp),
- ];
-
- let mut timestamp = None;
+ parse_timestamp_pattern(value, classify_timestamp_pattern(value),
eval_mode, tz)
+}
- // Iterate through patterns and try matching
- for (pattern, parse_func) in patterns {
- if pattern.is_match(value) {
- timestamp = parse_func(value, tz)?;
- break;
- }
- }
+/// Parses `value` according to the shape already determined for it by
+/// [`classify_timestamp_pattern`].
+///
+/// Both T-separator and space-separator date-time forms are supported.
Negative years are
+/// handled by `get_timestamp_values` detecting a leading '-'.
+fn parse_timestamp_pattern<T: TimeZone>(
+ value: &str,
+ pattern: Option<TimestampPattern>,
+ eval_mode: EvalMode,
+ tz: &T,
+) -> SparkResult<Option<i64>> {
+ let timestamp = match pattern {
+ Some(TimestampPattern::Year) => get_timestamp_values(value, "year",
tz)?,
+ Some(TimestampPattern::Month) => get_timestamp_values(value, "month",
tz)?,
+ Some(TimestampPattern::Day) => get_timestamp_values(value, "day", tz)?,
+ Some(TimestampPattern::Hour) => get_timestamp_values(value, "hour",
tz)?,
+ Some(TimestampPattern::Minute) => get_timestamp_values(value,
"minute", tz)?,
+ Some(TimestampPattern::Second) => get_timestamp_values(value,
"second", tz)?,
+ Some(TimestampPattern::Microsecond) => get_timestamp_values(value,
"microsecond", tz)?,
+ Some(_) => parse_str_to_time_only_timestamp(value, tz)?,
Review Comment:
Fixed in ad2e83017 — not a blocker, but you're describing a
silent-wrong-answer failure mode, which is the kind worth closing off while it
is cheap.
The catch-all is gone; the time-only arm now lists its variants:
```rust
// Listed out rather than caught by a `Some(_)` arm on purpose: a catch-all
would route
// any future non-time-only shape into the time-only parser and produce a
wrong value
// silently. Spelling out the variants makes adding one a compile error here.
Some(
TimestampPattern::TimeOnlyH
| TimestampPattern::TimeOnlyHm
| TimestampPattern::TimeOnlyHms
| TimestampPattern::TimeOnlyHmsu
| TimestampPattern::BareHm
| TimestampPattern::BareHms
| TimestampPattern::BareHmsu,
) => parse_str_to_time_only_timestamp(value, tz)?,
```
So adding a variant now stops the build here rather than quietly parsing a
date shape as a time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
