ninglinLiu commented on code in PR #7215:
URL: https://github.com/apache/datafusion-comet/pull/7215#discussion_r4379848463
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1564,52 +1566,123 @@ fn parse_sign_offset(s: &str) -> Option<i32> {
if rest.is_empty() || !rest.bytes().all(|b| b.is_ascii_digit() || b ==
b':') {
return None;
}
- let (h, m) = if let Some(colon_pos) = rest.find(':') {
+ let (h, m, sec) = if let Some(colon_pos) = rest.find(':') {
let h_str = &rest[..colon_pos];
- let m_str = &rest[colon_pos + 1..];
+ let mut m_str = &rest[colon_pos + 1..];
+ // "+HH:MM:SS": java.time.ZoneOffset accepts a seconds field with
two-digit minutes and
+ // seconds. Spark's `getZoneId` pads a one-digit hour first, so
"+8:00:00" is valid too.
+ let mut sec = 0;
+ if let Some(sec_pos) = m_str.find(':') {
+ let s_str = &m_str[sec_pos + 1..];
+ m_str = &m_str[..sec_pos];
+ if !(1..=2).contains(&h_str.len()) || m_str.len() != 2 ||
s_str.len() != 2 {
+ return None;
+ }
+ sec = s_str.parse::<i32>().ok()?;
+ }
if !(1..=2).contains(&h_str.len()) || !(1..=2).contains(&m_str.len()) {
return None;
}
let h: i32 = h_str.parse().ok()?;
- // Note: "+HH:MM:SS" (with seconds) is not handled; Spark accepts it
but it is rare.
let m: i32 = m_str.parse().ok()?;
- (h, m)
+ (h, m, sec)
} else {
match rest.len() {
- 1 | 2 => (rest.parse::<i32>().ok()?, 0),
+ 1 | 2 => (rest.parse::<i32>().ok()?, 0, 0),
4 => (
rest[..2].parse::<i32>().ok()?,
rest[2..].parse::<i32>().ok()?,
+ 0,
),
_ => return None,
}
};
- if !(0..=18).contains(&h) || !(0..=59).contains(&m) || (h == 18 && m != 0)
{
+ if !(0..=18).contains(&h)
+ || !(0..=59).contains(&m)
+ || !(0..=59).contains(&sec)
+ || (h == 18 && (m != 0 || sec != 0))
+ {
return None;
}
- Some(sign * (h * 3600 + m * 60))
+ Some(sign * (h * 3600 + m * 60 + sec))
}
/// Constructs a [`Tz`] from an offset measured in seconds.
/// E.g. `+7*3600 + 30*60` -> `"+07:30"`.
fn tz_from_offset_secs(secs: i32) -> Option<Tz> {
+ // `Tz` offsets have minute precision, so an offset with a non-zero
seconds part such as
+ // "+08:00:30" cannot be represented. Returning `None` leaves the value
unparsed, while
+ // Spark accepts it: https://github.com/apache/datafusion-comet/issues/7224
+ if secs % 60 != 0 {
+ return None;
+ }
let abs = secs.abs();
let h = abs / 3600;
let m = (abs % 3600) / 60;
let sign = if secs >= 0 { '+' } else { '-' };
Tz::from_str(&format!("{}{:02}:{:02}", sign, h, m)).ok()
}
-/// Returns the last (rightmost) byte position where `needle` starts inside
`haystack`.
-fn rfind_str(haystack: &str, needle: &str) -> Option<usize> {
- let hb = haystack.as_bytes();
- let nb = needle.as_bytes();
- if nb.len() > hb.len() {
+/// `java.time.ZoneId.SHORT_IDS`, which Spark passes to `ZoneId.of` when it
parses the zone
+/// of a timestamp string.
+const JAVA_SHORT_IDS: &[(&str, &str)] = &[
+ ("ACT", "Australia/Darwin"),
+ ("AET", "Australia/Sydney"),
+ ("AGT", "America/Argentina/Buenos_Aires"),
+ ("ART", "Africa/Cairo"),
+ ("AST", "America/Anchorage"),
+ ("BET", "America/Sao_Paulo"),
+ ("BST", "Asia/Dhaka"),
+ ("CAT", "Africa/Harare"),
+ ("CNT", "America/St_Johns"),
+ ("CST", "America/Chicago"),
+ ("CTT", "Asia/Shanghai"),
+ ("EAT", "Africa/Addis_Ababa"),
+ ("ECT", "Europe/Paris"),
+ ("IET", "America/Indiana/Indianapolis"),
+ ("IST", "Asia/Kolkata"),
+ ("JST", "Asia/Tokyo"),
+ ("MIT", "Pacific/Apia"),
+ ("NET", "Asia/Yerevan"),
+ ("NST", "Pacific/Auckland"),
+ ("PLT", "Asia/Karachi"),
+ ("PNT", "America/Phoenix"),
+ ("PRT", "America/Puerto_Rico"),
+ ("PST", "America/Los_Angeles"),
+ ("SST", "Pacific/Guadalcanal"),
+ ("VST", "Asia/Ho_Chi_Minh"),
+ ("EST", "-05:00"),
+ ("MST", "-07:00"),
+ ("HST", "-10:00"),
+];
+
+/// Resolves a zone id that starts with a letter the way `ZoneId.of(id,
SHORT_IDS)` does:
+/// a `SHORT_IDS` alias first, then a tz database region id such as `CET`,
`Zulu` or
+/// `Etc/GMT+8`. Prefix forms such as `UTC+07:30` are not handled here.
+fn zone_id_to_tz(id: &str) -> Option<Tz> {
+ if !id.starts_with(|c: char| c.is_ascii_alphabetic()) {
return None;
}
- (0..=(hb.len() - nb.len()))
- .rev()
- .find(|&i| hb[i..].starts_with(nb))
+ if let Some((_, target)) = JAVA_SHORT_IDS.iter().find(|(alias, _)| *alias
== id) {
+ return Tz::from_str(target).ok();
+ }
+ Tz::from_str(id).ok()
+}
+
+/// Looks for a zone id at the end of `value` that starts right after a digit
or whitespace,
+/// e.g. "2020-01-01 12:34:56 CET" or "2020-01-01
12:34:56America/Los_Angeles", and returns
+/// the text before it together with the resolved zone.
+fn extract_zone_id_suffix(value: &str) -> Option<(&str, Tz)> {
+ let bytes = value.as_bytes();
+ (1..bytes.len()).rev().find_map(|pos| {
+ let prev = bytes[pos - 1];
+ if !bytes[pos].is_ascii_alphabetic()
+ || !(prev.is_ascii_digit() || prev.is_ascii_whitespace())
+ {
+ return None;
+ }
+ zone_id_to_tz(&value[pos..]).map(|tz| (&value[..pos], tz))
Review Comment:
Thanks, that was a real cost. Added `could_be_zone_id` in d0ef68b, checked
before any lookup: the candidate must start with an upper-case letter and be at
most 32 bytes of `[A-Za-z0-9_+/-]`. All 597 chrono-tz 0.10.4 ids and the 28
aliases have that shape, and ids are case sensitive, so nothing that could
resolve is skipped. For `not a timestamp`, `a timestamp` fails on the space and
`timestamp` on the lower-case start, so no `Tz::from_str` call is made.
`cargo bench --bench cast_string_to_timestamp -- invalid` (criterion median,
same machine, `--measurement-time 4`):
| case | before this PR (bb76f12) | PR at 3605746 | with the guard (d0ef68b)
|
|---|---|---|---|
| legacy | 1.071 ms | 1.825 ms | 0.368 ms |
| try | 1.176 ms | 1.916 ms | 0.357 ms |
| non_utc/legacy | 1.089 ms | 1.990 ms | 0.366 ms |
| non_utc/try | 1.090 ms | 2.071 ms | 0.366 ms |
| ntz/legacy | 1.069 ms | 1.951 ms | 0.348 ms |
| ntz/try | 1.111 ms | 1.821 ms | 0.343 ms |
So the probes cost about 75% on this case, and with the guard it is below
the pre-PR number. I did not dig into why it ends up lower than before; it is a
shared box, so treat the last column as roughly "no regression" rather than a
speedup.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1564,52 +1566,123 @@ fn parse_sign_offset(s: &str) -> Option<i32> {
if rest.is_empty() || !rest.bytes().all(|b| b.is_ascii_digit() || b ==
b':') {
return None;
}
- let (h, m) = if let Some(colon_pos) = rest.find(':') {
+ let (h, m, sec) = if let Some(colon_pos) = rest.find(':') {
let h_str = &rest[..colon_pos];
- let m_str = &rest[colon_pos + 1..];
+ let mut m_str = &rest[colon_pos + 1..];
+ // "+HH:MM:SS": java.time.ZoneOffset accepts a seconds field with
two-digit minutes and
+ // seconds. Spark's `getZoneId` pads a one-digit hour first, so
"+8:00:00" is valid too.
+ let mut sec = 0;
+ if let Some(sec_pos) = m_str.find(':') {
+ let s_str = &m_str[sec_pos + 1..];
+ m_str = &m_str[..sec_pos];
+ if !(1..=2).contains(&h_str.len()) || m_str.len() != 2 ||
s_str.len() != 2 {
+ return None;
+ }
+ sec = s_str.parse::<i32>().ok()?;
+ }
if !(1..=2).contains(&h_str.len()) || !(1..=2).contains(&m_str.len()) {
return None;
}
let h: i32 = h_str.parse().ok()?;
- // Note: "+HH:MM:SS" (with seconds) is not handled; Spark accepts it
but it is rare.
let m: i32 = m_str.parse().ok()?;
- (h, m)
+ (h, m, sec)
} else {
match rest.len() {
- 1 | 2 => (rest.parse::<i32>().ok()?, 0),
+ 1 | 2 => (rest.parse::<i32>().ok()?, 0, 0),
4 => (
rest[..2].parse::<i32>().ok()?,
rest[2..].parse::<i32>().ok()?,
+ 0,
),
_ => return None,
Review Comment:
Accepted in d0ef68b: the colon-free branch now takes 6 digits as `+HHMMSS`,
through the same seconds handling, so `+080000`, `-080000`, `UTC-080000` and
`GMT+053000` resolve, and `+080030` still returns NULL like the other
non-zero-seconds forms in #7224. Added unit cases for these and `+080000` /
`UTC-080000` rows to the fixture, and the docs list now names `+HHMMSS`.
##########
spark/src/test/resources/sql-tests/expressions/cast/cast_string_to_timestamp_zone_ids.sql:
##########
@@ -0,0 +1,78 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Zone suffixes that Spark resolves through ZoneId.of(id, ZoneId.SHORT_IDS):
SHORT_IDS
+-- aliases, region ids and offsets with a seconds field. Summer dates separate
the aliases
+-- that map to a region (PST follows PDT) from the fixed-offset ones (EST
stays -05:00).
+-- Config: spark.sql.ansi.enabled=false
+-- ConfigMatrix: spark.sql.session.timeZone=UTC,America/Los_Angeles
+
+statement
+CREATE TABLE zone_id_valid(s string) USING parquet
+
+statement
+INSERT INTO zone_id_valid VALUES
+ ('2020-01-01 12:34:56 PST'), ('2020-07-01 12:34:56 PST'),
Review Comment:
Done in d0ef68b: the table now holds all 28 aliases, each with `2020-01-15`
and `2020-07-15`, so Spark is the reference for every target, including the
DST-observing ones. It passes in both session time zones locally. I also
removed the six literal queries from `cast.sql` and the two literal queries
here, since the table covers the same values through the same comparison. The
ANSI `expect_error` queries at the end stay.
--
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]
