comphead commented on code in PR #7215: URL: https://github.com/apache/datafusion-comet/pull/7215#discussion_r4379460202
########## 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: `java_short_ids_resolve_test` checks that each alias resolves and that there are 28, so a wrong target such as `AST` mapped to another region would still pass. This table is the one place Spark checks the mapping, and it covers `PST`, `EST`, `CST`, `JST` and `HST`. Could it hold all 28 aliases, each with a January and a July date? That makes Spark the reference for the DST-observing ones (`AET`, `ART`, `AST`, `BET`, `CNT`, `ECT`, `NST`). In the other direction, the six literal queries added to `cast.sql` and the two at the end of this file repeat values already in this table, which runs through the same Spark comparison for `timestamp`, `timestamp_ntz` and `try_cast` in both session time zones, so they could go unless one is kept on purpose for the scalar path. ########## 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: This probes `Tz::from_str` at every position where a letter follows a digit or whitespace, and arrow-array 59.3.1's `Tz::from_str` builds an `ArrowError` with `format!` on every miss. `extract_offset_suffix` runs whenever no base pattern matches, so it runs for every malformed value. Before this PR such a value did no zone lookup unless its last word held a `/`. Now `not a timestamp`, the `invalid` case in `benches/cast_string_to_timestamp.rs`, probes `a timestamp` and `timestamp` and allocates two error strings, and a long malformed string pays one probe per word. Could you add before and after numbers for that case? A cheap guard before the lookup would skip a candidate longer than the longest tz id (32 bytes in chrono-tz 0.10.4) or holding a character outside `[A-Za-z0-9_+/-]`. I have not run the benchmark. ########## 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: Spark's `getZoneId` also accepts `+hhmmss` and `UTC+hhmmss` (`ZoneId.of("+080000")` is `+08:00`), but the colon-free branch here takes only 1, 2 or 4 digits. The new doc list names the supported forms and says only non-zero seconds are unsupported, so this gap is not visible to users. Could a zero-seconds `+HHMMSS` be accepted with the seconds handling you added, or be listed in #7224 and the docs? This predates the PR. -- 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]
