andygrove commented on code in PR #5130:
URL: https://github.com/apache/datafusion-comet/pull/5130#discussion_r3690518753
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1619,87 +1574,307 @@ 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.
Review Comment:
Agreed, that reads much better — a shape and its pattern are declared
together instead of relying on two arrays staying in the same order. Applied in
e14e086, with `ALL` and `regex_str` on the enum and the `RegexSet` built from
`ALL.map(regex_str)`.
I kept the `Year` 4-6 vs `\d{4,7}` note on `regex_str` since that asymmetry
is the easiest thing to lose when editing the patterns. The differential test
now builds its reference regexes from `regex_str()` too, and I re-confirmed it
still fails when the classifier is perturbed.
--
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]
