comphead commented on code in PR #7579:
URL: https://github.com/apache/datafusion-comet/pull/7579#discussion_r4532818329
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1538,15 +1538,33 @@ fn timestamp_parser<T: TimeZone>(
// base pattern. This prevents the '-' in plain date strings like
"2015-03-18"
// from being misidentified as a negative-offset sign.
if pattern.is_none() {
- if let Some((stripped, suffix_tz)) = extract_offset_suffix(value) {
+ if let Some((stripped, suffix_tz, residual_secs)) =
extract_offset_suffix(value) {
// Spark applies Java String.trim to the zone, not Unicode
whitespace trimming.
let stripped = stripped.trim_end_matches(|c: char| c <= '\u{20}');
// A zone suffix is only meaningful after the seconds segment.
Otherwise fall
// through with the unstripped value, which no base pattern
matches, so it is
// reported as malformed (null, or CAST_INVALID_INPUT under ANSI)
like Spark does.
let stripped_pattern = classify_timestamp_pattern(stripped);
if
stripped_pattern.is_some_and(TimestampPattern::ends_with_seconds_segment) {
- return parse_timestamp_pattern(stripped, stripped_pattern,
eval_mode, &suffix_tz);
+ let parsed =
+ parse_timestamp_pattern(stripped, stripped_pattern,
eval_mode, &suffix_tz)?;
+ // Arrow's `Tz` holds whole minutes, so an offset such as
`+08:00:30` arrives as
+ // the `+08:00` zone plus a 30-second residual. Subtract the
residual to land on
+ // the exact instant Spark computes. An instant pushed below
the representable
+ // range is out of range, which Spark reports like any other
invalid value.
+ let adjusted = parsed
+ .and_then(|micros|
micros.checked_sub(i64::from(residual_secs) * 1_000_000));
+ return match adjusted {
+ Some(micros) => Ok(Some(micros)),
+ None if eval_mode == EvalMode::Ansi => {
+ Err(SparkError::InvalidInputInCastToDatetime {
+ value: value.to_string(),
+ from_type: "STRING".to_string(),
+ to_type: "TIMESTAMP".to_string(),
+ })
+ }
+ None => Ok(None),
+ };
Review Comment:
This is the third copy of the "ANSI error, otherwise NULL" block in the
`TIMESTAMP` path, next to the empty-input check at the top of
`timestamp_parser` and the end of `parse_timestamp_pattern`.
`invalid_ntz_timestamp` already does this for `TIMESTAMP_NTZ`. Could a
`TIMESTAMP` sibling, or `invalid_ntz_timestamp` taking the target type, replace
all three? This arm would then be `None => invalid_timestamp(value, eval_mode)`.
##########
spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala:
##########
@@ -1959,6 +1965,8 @@ class CometNativeCastSuite
"2020-06-15T12:30:00+05:30",
"2020-06-15T12:30:00-08:00",
"2020-06-15T12:30:00 UTC",
+ "2020-06-15T12:30:00+08:00:30",
+ "2020-06-15T12:30:00-01:30:15",
Review Comment:
Could this list also have the rejected `2020-06-15T12:30:00+8:0:3`? Spark
validates the zone for `TIMESTAMP_NTZ` too (`parseTimestampString` calls
`getZoneId` before the zone is dropped), so the NTZ path returns NULL for it,
and that is worth pinning here as well as in the `TIMESTAMP` list.
--
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]
