sunchao commented on code in PR #5682:
URL: https://github.com/apache/datafusion-comet/pull/5682#discussion_r3964548535
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1636,34 +1641,58 @@ fn extract_offset_suffix(value: &str) -> Option<(&str,
Tz)> {
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());
+// These shapes transcribe the per-segment digit rules of Spark's
+// `SparkDateTimeUtils.parseTimestampString` (`isValidDigits`): the year takes
4-6 digits
+// (`maxDigitsYear = 6`, so "0002020-01-01" is malformed for a timestamp even
though
+// `stringToDate`, ported by `date_parser`, allows 7),
month/day/hour/minute/second take 1-2
+// digits each, and the fraction takes any number of digits including none
("12:34:56." is
+// valid), of which only the first six are kept. All digits must be ASCII,
matching Spark's
+// byte scanner and the numeric parsers used after shape recognition.
+// Keep the ASCII ranges: Unicode `\d` also costs substantially more to match
on valid input.
+static RE_YEAR: LazyLock<Regex> = LazyLock::new(||
Regex::new(r"^-?[0-9]{4,6}$").unwrap());
+static RE_MONTH: LazyLock<Regex> =
+ LazyLock::new(|| Regex::new(r"^-?[0-9]{4,6}-[0-9]{1,2}$").unwrap());
Review Comment:
[P1] Guard offset parsing against mixed-width Unicode digits
`2020-1१` (ASCII `1` followed by U+0967, a three-byte digit) matched the
base's `\d{2}` month pattern, but no longer matches this ASCII pattern. Both
timestamp parsers then try the date's `-` as an offset sign.
`parse_sign_offset` sees four bytes after it and `rest[..2]` splits the Unicode
character, panicking before Legacy/TRY can return NULL or ANSI can raise
CAST_INVALID_INPUT. The exact head array cast panics for both TIMESTAMP and
TIMESTAMP_NTZ in all three modes; Spark 4.1.3 rejects the input. Please reject
non-ASCII offset bytes before fixed-width slicing (or use checked ranges), and
add Parquet-backed column regressions for mixed ASCII/three-byte Unicode month
and day digits. The current two-byte Unicode fixtures do not exercise this path.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1470,7 +1468,14 @@ fn timestamp_parser<T: TimeZone>(
if !has_direct_match {
if let Some((stripped, suffix_tz)) = extract_offset_suffix(value) {
- return timestamp_parser_with_tz(stripped, eval_mode, &suffix_tz);
+ // Spark applies Java String.trim to the zone, not Unicode
whitespace trimming.
+ let stripped = stripped.trim_end_matches(|c: char| c <= '\u{20}');
Review Comment:
[P2] Reject malformed numeric offsets newly admitted by the prefix trim
`2020-01-01 12:34:56 +08:000` now has its space removed here, passes
`ends_with_seconds_segment`, and returns `1577853296000000` as TIMESTAMP. The
base returns NULL (or an ANSI error), as does Spark 4.1.3; `+008:00` and
`+18:01` have the same new behavior. `parse_sign_offset` parses colon-separated
parts without a 1–2 digit limit and allows 18 hours plus nonzero minutes before
normalizing to an Arrow offset. Could we validate the numeric zone's field
widths and ±18:00 bound before accepting it, with separate column inputs under
Legacy, TRY and ANSI? This newly accepted spaced TIMESTAMP case is the
regression here; unspaced malformed offsets and NTZ acceptance already existed
on the base.
--
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]
