sunchao commented on code in PR #5682:
URL: https://github.com/apache/datafusion-comet/pull/5682#discussion_r3935745419
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1636,34 +1641,52 @@ 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.
+// 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.
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_MONTH: LazyLock<Regex> = LazyLock::new(||
Regex::new(r"^-?\d{4,6}-\d{1,2}$").unwrap());
Review Comment:
### Correctness
[P2] Restrict newly accepted timestamp segments to ASCII digits
Could we make the broadened digit classes ASCII-only and add a column-input
regression? Rust's `\d` also matches Unicode decimal digits. For example,
`2020-٢` (U+0662) now matches `RE_MONTH`, then `parse_to_timestamp_info`
substitutes month 1 when `parse::<u32>()` fails. The production Arrow entry
points return `1577836800000000` for both TIMESTAMP and TIMESTAMP_NTZ in
Legacy, Try and ANSI at this head. The base returns null in Legacy/Try and an
error in ANSI. Spark 3.5 and 4.0 reject the input because their scanner accepts
only ASCII `0` through `9`.
The newly accepted single-digit day, minute and second forms have the same
problem. Using `[0-9]` for these shapes preserves the intended one-digit
support without silently converting malformed values to valid timestamps.
Please exercise the regression with column input so Spark constant folding
cannot bypass the native cast. This finding excludes the pre-existing two-digit
Unicode and hour-only cases.
--
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]
