sunchao commented on code in PR #5130:
URL: https://github.com/apache/datafusion-comet/pull/5130#discussion_r4104259257
##########
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:
[P2] Could we bound digit scans for fixed-width fields? For a string column
containing `repeat('1', 8192) || 'Z'`, a legacy timestamp cast should reject
the oversized year promptly and return NULL. The result remains NULL, but
`digit_run` now traverses all 8,192 digits both before and after stripping `Z`.
The base regexes reject after a bounded prefix. Release-mode measurements
reproduce a 12.5–12.9× parser slowdown, reducing throughput for batches
containing these malformed values. Stop after the maximum field width plus one
for year/month/day/hour/minute/second, retain unbounded scanning for fractional
digits, and include this shape in the benchmark.
Evidence: A disposable Rust harness copied the timestamp parser bodies from
the requested base and head and built both with optimized dependencies (`regex`
1.13.1, `chrono` 0.4.45, Arrow 59.3.0). It called `timestamp_parser` with UTC,
Legacy mode, and `is_spark4_plus=false`, using 20 warm-up calls and 10,000
timed calls per sample. Across two independent processes with three samples
each, 8,192 ASCII digits followed by `Z` took approximately 431–444 ns/call at
base versus 5,533–5,578 ns/call at head. With 1,024 digits, the regression was
approximately 1.85–1.95×. A disposable variant bounding fixed-field scans to
eight bytes while preserving unrestricted fraction validation reduced the
8,192-digit case to approximately 374–414 ns/call. Reproduction source and logs
are under `/tmp/comet-5130-review/perf/`, `/tmp/comet-5130-review/perf.log`,
and `/tmp/comet-5130-review/perf-second.log`.
--
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]
