rich7420 commented on code in PR #5130:
URL: https://github.com/apache/datafusion-comet/pull/5130#discussion_r3951634904
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -2941,6 +3195,532 @@ mod tests {
);
}
+ /// Days from 1970-01-01 for a proleptic-Gregorian civil date (Howard
Hinnant's algorithm).
+ ///
+ /// Used to compute the expected values of the ported Spark tests below
independently of
+ /// the parser under test. Handles the full year range Spark supports,
including negative
+ /// years and years beyond `chrono`'s limits, which the
`Long.MinValue`/`MaxValue`
+ /// boundary cases need.
+ fn days_from_civil(year: i64, month: i64, day: i64) -> i64 {
+ let y = year - i64::from(month <= 2);
+ // Truncating division, as the algorithm requires - not Rust's
`%`-consistent one.
+ let era = if y >= 0 { y } else { y - 399 } / 400;
+ let year_of_era = y - era * 400; // [0, 399]
+ let day_of_year = (153 * (month + if month > 2 { -3 } else { 9 }) + 2)
/ 5 + day - 1;
+ let day_of_era = year_of_era * 365 + year_of_era / 4 - year_of_era /
100 + day_of_year;
+ era * 146097 + day_of_era - 719468
+ }
+
+ /// Microseconds since the epoch for a UTC civil date-time. The Rust
equivalent of the
+ /// `DateTimeTestUtils.date(..., zid = UTC)` helper the Spark tests below
use.
+ ///
+ /// Computed in `i128` because the `Long.MinValue` boundary case below is
exactly
+ /// `i64::MIN` microseconds, and its intermediate whole-second product
overflows `i64`.
+ fn utc_micros(
+ year: i64,
+ month: i64,
+ day: i64,
+ hour: i64,
+ minute: i64,
+ second: i64,
+ micros: i64,
+ ) -> i64 {
+ let seconds = i128::from(days_from_civil(year, month, day)) * 86_400
+ + i128::from(hour) * 3_600
+ + i128::from(minute) * 60
+ + i128::from(second);
+ i64::try_from(seconds * 1_000_000 + i128::from(micros))
+ .expect("expected value should fit in i64")
+ }
+
+ /// Reinterprets a UTC-computed instant as a wall-clock time in a fixed
zone
+ /// `offset_seconds` east of UTC. The equivalent of `date(..., zid =
getZoneId("+07:30"))`.
+ fn at_offset(utc_micros: i64, offset_seconds: i64) -> i64 {
+ utc_micros - offset_seconds * 1_000_000
+ }
+
+ /// The inputs for which Comet's ANSI mode returns null where Spark raises
+ /// CAST_INVALID_INPUT: a leading `+` that is not followed by a
`<digits>-` year.
+ ///
+ /// This predicate exists only to keep the ported Spark tests below honest
about the
+ /// divergence rather than skipping the ANSI dimension for them entirely.
Tracked by
+ /// <https://github.com/apache/datafusion-comet/issues/5165>.
+ fn ansi_returns_null_instead_of_raising(value: &str) -> bool {
+ let trimmed = value.trim();
+ match trimmed.strip_prefix('+') {
+ None => false,
+ Some(rest) => !matches!(
+ rest.find(|c: char| !c.is_ascii_digit()),
+ Some(i) if i >= 1 && rest.as_bytes()[i] == b'-'
+ ),
+ }
+ }
+
+ /// Asserts `value` parses to `expected` in every eval mode, under session
timezone `tz`.
+ ///
+ /// Mirrors `checkStringToTimestamp` in Spark's `DateTimeUtilsSuite`,
extended over the
+ /// eval modes Comet has: a `None` expectation additionally requires ANSI
mode to raise
+ /// rather than return null.
+ fn check_string_to_timestamp(value: &str, expected: Option<i64>, tz: &str)
{
+ let tz = &Tz::from_str(tz).unwrap();
+ for is_spark4_plus in [false, true] {
+ for eval_mode in [EvalMode::Legacy, EvalMode::Try] {
+ assert_eq!(
+ timestamp_parser(value, eval_mode, tz,
is_spark4_plus).unwrap(),
+ expected,
+ "{value:?} in {eval_mode:?} (spark4={is_spark4_plus})"
+ );
+ }
+ let ansi = timestamp_parser(value, EvalMode::Ansi, tz,
is_spark4_plus);
+ match expected {
+ Some(_) => assert_eq!(
+ ansi.unwrap(),
+ expected,
+ "{value:?} in ANSI (spark4={is_spark4_plus})"
+ ),
+ // Spark raises CAST_INVALID_INPUT under ANSI for anything it
cannot parse,
+ // except that an all-whitespace or empty string is null in
every mode.
+ None if value.trim().is_empty() => {
+ assert_eq!(ansi.unwrap(), None, "{value:?} in ANSI")
+ }
+ // Pre-existing gap, not introduced by the shape classifier: a
leading '+'
+ // that is not a year sign returns null in every mode, where
Spark raises
+ // under ANSI. Tracked by issue 5165; asserted here so it
cannot widen.
+ None if ansi_returns_null_instead_of_raising(value) => {
+ assert_eq!(ansi.unwrap(), None, "{value:?} in ANSI")
+ }
+ None => assert!(
+ ansi.is_err(),
+ "{value:?} should raise in ANSI (spark4={is_spark4_plus})"
+ ),
+ }
+ }
+ }
+
+ /// Port of `test("string to timestamp")` in Spark's `DateTimeUtilsSuite`.
+ ///
+ /// Spark runs its version over `ALL_TIMEZONES`; the session-timezone
dimension is covered
+ /// separately by `spark_string_to_timestamp_session_timezone_test` below,
so this pins the
+ /// exact expected microseconds under a UTC session timezone.
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn spark_string_to_timestamp_test() {
+ let check =
+ |value: &str, expected: Option<i64>|
check_string_to_timestamp(value, expected, "UTC");
+
+ check(
+ "1969-12-31 16:00:00",
+ Some(utc_micros(1969, 12, 31, 16, 0, 0, 0)),
+ );
+ check("0001", Some(utc_micros(1, 1, 1, 0, 0, 0, 0)));
+ check("2015-03", Some(utc_micros(2015, 3, 1, 0, 0, 0, 0)));
+ for value in ["2015-03-18", "2015-03-18 ", " 2015-03-18", " 2015-03-18
"] {
+ check(value, Some(utc_micros(2015, 3, 18, 0, 0, 0, 0)));
+ }
+
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 0));
+ check("2015-03-18 12:03:17", expected);
+ check("2015-03-18T12:03:17", expected);
+
+ // When the string carries a timezone, that zone wins over the session
timezone.
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 0),
+ -(13 * 3600 + 53 * 60),
+ ));
+ check("2015-03-18T12:03:17-13:53", expected);
+ check("2015-03-18T12:03:17GMT-13:53", expected);
+ check("2015-03-18T12:03:17-1353", expected);
+
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 0));
+ check("2015-03-18T12:03:17Z", expected);
+ check("2015-03-18 12:03:17Z", expected);
+ check("2015-03-18 12:03:17UTC", expected);
+
+ let expected = Some(at_offset(utc_micros(2015, 3, 18, 12, 3, 17, 0),
-3600));
+ check("2015-03-18T12:03:17-1:0", expected);
+ check("2015-03-18T12:03:17-01:00", expected);
+ check("2015-03-18T12:03:17GMT-01:00", expected);
+ check("2015-03-18T12:03:17-0100", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 0),
+ 7 * 3600 + 30 * 60,
+ ));
+ check("2015-03-18T12:03:17+07:30", expected);
+ check("2015-03-18T12:03:17 GMT+07:30", expected);
+ check("2015-03-18T12:03:17+0730", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 0),
+ 7 * 3600 + 3 * 60,
+ ));
+ check("2015-03-18T12:03:17+07:03", expected);
+ check("2015-03-18T12:03:17GMT+07:03", expected);
+ check("2015-03-18T12:03:17+0703", expected);
+
+ // Strings including milliseconds.
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 123_000));
+ check("2015-03-18 12:03:17.123", expected);
+ check("2015-03-18T12:03:17.123", expected);
+
+ let expected = Some(utc_micros(2015, 3, 18, 12, 3, 17, 456_000));
+ check("2015-03-18T12:03:17.456Z", expected);
+ check("2015-03-18 12:03:17.456Z", expected);
+ check("2015-03-18 12:03:17.456 UTC", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_000),
+ -3600,
+ ));
+ check("2015-03-18T12:03:17.123-1:0", expected);
+ check("2015-03-18T12:03:17.123-01:00", expected);
+ check("2015-03-18T12:03:17.123 GMT-01:00", expected);
+ check("2015-03-18T12:03:17.123-0100", expected);
+
+ let plus_730 = 7 * 3600 + 30 * 60;
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_000),
+ plus_730,
+ ));
+ check("2015-03-18T12:03:17.123+07:30", expected);
+ check("2015-03-18T12:03:17.123 GMT+07:30", expected);
+ check("2015-03-18T12:03:17.123+0730", expected);
+ check("2015-03-18T12:03:17.123GMT+07:30", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_121),
+ plus_730,
+ ));
+ check("2015-03-18T12:03:17.123121+7:30", expected);
+ check("2015-03-18T12:03:17.123121 GMT+0730", expected);
+
+ let expected = Some(at_offset(
+ utc_micros(2015, 3, 18, 12, 3, 17, 123_120),
+ plus_730,
+ ));
+ check("2015-03-18T12:03:17.12312+7:30", expected);
+ check("2015-03-18T12:03:17.12312 UT+07:30", expected);
+ check("2015-03-18T12:03:17.12312+0730", expected);
+
+ // Time-only strings are anchored to the current date, so assert
against a
+ // today-relative expectation rather than a fixed instant.
+ let today = chrono::Utc::now().date_naive();
Review Comment:
Could we use the input timezone's current date here? After 16:30 UTC, the
`+07:30` cases are a day ahead of UTC, so the expected value is off by 24
hours. I reproduced the failure and confirmed that using the date in `+07:30`
fixes it.
--
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]
