andygrove commented on code in PR #5130:
URL: https://github.com/apache/datafusion-comet/pull/5130#discussion_r4144071002
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1634,87 +1589,322 @@ fn extract_offset_suffix(value: &str) -> Option<(&str,
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())
Review Comment:
Fixed in 5833e7f8f. The fixed-width segments now count at most one digit
past their widest valid length, which is enough to reject an overlong run: 7
for the year and 3 for month, day, hour, minute and second (`MAX_YEAR_DIGITS +
1` / `MAX_SEGMENT_DIGITS + 1`). The fraction is still scanned in full, since it
may be any length. After the latest main merge these limits follow #5682's
segment rules (4-6 year digits, 1-2 digits for every other segment).
I added a `long_digits_zone` batch (1,024 digits then `Z`) to
`cast_string_to_timestamp.rs`, 8,192-digit cases to
`test_timestamp_pattern_segment_rules`, and two long seeds to the regex
differential test.
Criterion on a 32-core x86_64 box, legacy mode, per 8,192-row batch:
| case | main (regexes) | before the fix | with the fix |
| --- | --- | --- | --- |
| `timestamp/long_digits_zone` | 2133 µs | 7770 µs | 468 µs |
| `timestamp_ntz/long_digits_zone` | 1806 µs | 7814 µs | 451 µs |
| `timestamp/canonical` | 2005 µs | 483 µs | 474 µs |
| `timestamp/single_digit_segments` | 1953 µs | 458 µs | 434 µs |
The typical shapes are unchanged or slightly faster with the bound.
--
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]
