This is an automated email from the ASF dual-hosted git repository.
andygrove pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
The following commit(s) were added to refs/heads/main by this push:
new a74839cf76 fix: match Spark whitespace trimming in to_time and
try_to_time (#5364)
a74839cf76 is described below
commit a74839cf76ff15a7de4128b4740e8133ae8d0d29
Author: Chao Sun <[email protected]>
AuthorDate: Sat Aug 15 12:24:17 2026 -0700
fix: match Spark whitespace trimming in to_time and try_to_time (#5364)
* fix: match Spark whitespace trimming in to_time and try_to_time
* test: address to_time trim review feedback
---
.../user-guide/latest/compatibility/index.md | 8 +-
native/spark-expr/src/conversion_funcs/mod.rs | 2 +-
native/spark-expr/src/conversion_funcs/trim.rs | 6 +-
native/spark-expr/src/datetime_funcs/to_time.rs | 245 ++++++++++++++++++---
.../sql-tests/expressions/datetime/to_time.sql | 77 +++++++
5 files changed, 306 insertions(+), 32 deletions(-)
diff --git a/docs/source/user-guide/latest/compatibility/index.md
b/docs/source/user-guide/latest/compatibility/index.md
index ae29a67bc1..001ec87606 100644
--- a/docs/source/user-guide/latest/compatibility/index.md
+++ b/docs/source/user-guide/latest/compatibility/index.md
@@ -134,10 +134,10 @@ so users hunting an unexpected value have a single place
to check:
`DECIMAL(1, 1)`) throws `NUMERIC_VALUE_OUT_OF_RANGE` regardless of the eval
mode. Spark returns
`NULL` under legacy and try mode, and only throws under ANSI
([#5068](https://github.com/apache/datafusion-comet/issues/5068)).
-- `CAST(string AS timestamp)`, `CAST(string AS timestamp_ntz)`, `to_time` and
`try_to_time` trim
- Unicode whitespace. Spark trims only the bytes `0x00`-`0x20` and `0x7F`, so
a value padded with
- an ASCII control byte parses in Spark and returns `NULL` in Comet, while a
value padded with
- non-ASCII whitespace such as `U+3000` returns `NULL` in Spark and parses in
Comet
+- `CAST(string AS timestamp)` and `CAST(string AS timestamp_ntz)` trim Unicode
whitespace.
+ Spark trims only the bytes `0x00`-`0x20` and `0x7F`, so a value padded with
an ASCII control byte
+ parses in Spark and returns `NULL` in Comet, while a value padded with
non-ASCII whitespace such
+ as `U+3000` returns `NULL` in Spark and parses in Comet
([#5149](https://github.com/apache/datafusion-comet/issues/5149)).
- Native `RANGE` window frames with an explicit `PRECEDING` / `FOLLOWING`
offset diverge from
Spark when the boundary arithmetic overflows for `DATE` or `DECIMAL` `ORDER
BY` columns
diff --git a/native/spark-expr/src/conversion_funcs/mod.rs
b/native/spark-expr/src/conversion_funcs/mod.rs
index 2f42c316b3..7b864e193b 100644
--- a/native/spark-expr/src/conversion_funcs/mod.rs
+++ b/native/spark-expr/src/conversion_funcs/mod.rs
@@ -20,5 +20,5 @@ pub mod cast;
mod numeric;
mod string;
mod temporal;
-mod trim;
+pub(crate) mod trim;
mod utils;
diff --git a/native/spark-expr/src/conversion_funcs/trim.rs
b/native/spark-expr/src/conversion_funcs/trim.rs
index 983bf34fa4..8cd7159096 100644
--- a/native/spark-expr/src/conversion_funcs/trim.rs
+++ b/native/spark-expr/src/conversion_funcs/trim.rs
@@ -31,9 +31,11 @@
//! `Double.parseDouble` trims before parsing, and
`Decimal.stringToJavaBigDecimal` does
//! `str.toString.trim`.
//!
+//! `to_time` and `try_to_time` also use [`trim_all`] after detecting an
optional AM/PM suffix;
+//! suffix detection itself first removes ASCII spaces only, matching Spark's
`stringToTime`.
+//!
//! \* `timestamp` and `timestamp_ntz` are listed for what Spark does; the
Comet parsers for those
-//! two targets have not been migrated to these helpers and still use
`str::trim`, as do `to_time`
-//! and `try_to_time` in `datetime_funcs::to_time`
+//! two targets have not been migrated to these helpers and still use
`str::trim`
//! (<https://github.com/apache/datafusion-comet/issues/5149>).
/// True for the bytes trimmed by
`org.apache.spark.unsafe.types.UTF8String.trimAll`, i.e. the
diff --git a/native/spark-expr/src/datetime_funcs/to_time.rs
b/native/spark-expr/src/datetime_funcs/to_time.rs
index 727998fdf8..249be04a4f 100644
--- a/native/spark-expr/src/datetime_funcs/to_time.rs
+++ b/native/spark-expr/src/datetime_funcs/to_time.rs
@@ -15,6 +15,7 @@
// specific language governing permissions and limitations
// under the License.
+use crate::conversion_funcs::trim::trim_all;
use arrow::array::{Array, StringArray, Time64NanosecondArray};
use datafusion::common::{DataFusionError, Result};
use datafusion::physical_plan::ColumnarValue;
@@ -80,26 +81,18 @@ pub fn spark_to_time(args: &[ColumnarValue], fail_on_error:
bool) -> Result<Colu
/// Parse a time string to nanoseconds from midnight, matching Spark's
stringToTime behavior.
/// Returns None for invalid input.
fn string_to_time(s: &str) -> Option<i64> {
- let trimmed = s.trim();
- if trimmed.is_empty() {
- return None;
- }
-
- // Spark's parseTimestampString gates the T-prefix branch on j == 0 (start
of
- // the trimmed string), so " T12:30" is rejected even though leading
whitespace
- // is trimmed: the original segment start differs from the trimmed
position.
- if trimmed.as_bytes()[0] == b'T' && s.as_bytes()[0].is_ascii_whitespace() {
- return None;
- }
-
- let bytes = trimmed.as_bytes();
- let num_chars = bytes.len();
-
- // Detect AM/PM suffix
- let (is_am, is_pm, has_suffix) = if num_chars > 2 {
- let last = bytes[num_chars - 1];
+ // Spark's stringToTime calls UTF8String.trimRight before looking for
AM/PM.
+ // Unlike trimAll, trimRight removes ASCII spaces only, so a control byte
+ // after AM/PM prevents the suffix from being recognized.
+ let right_trimmed = s.trim_end_matches(' ');
+ let bytes = right_trimmed.as_bytes();
+ let num_bytes = bytes.len();
+
+ // ASCII AM/PM suffix bytes cannot be UTF-8 continuation bytes, so byte
indexing is safe.
+ let (is_am, is_pm, has_suffix) = if num_bytes > 2 {
+ let last = bytes[num_bytes - 1];
if last == b'M' || last == b'm' {
- let second_last = bytes[num_chars - 2];
+ let second_last = bytes[num_bytes - 2];
let am = second_last == b'A' || second_last == b'a';
let pm = second_last == b'P' || second_last == b'p';
(am, pm, am || pm)
@@ -110,14 +103,24 @@ fn string_to_time(s: &str) -> Option<i64> {
(false, false, false)
};
- // Strip AM/PM suffix (and optional space before it)
- let time_str = if has_suffix {
- let end = num_chars - 2;
- let s = &trimmed[..end];
- s.trim_end()
+ // Spark passes the remaining segment to parseTimestampString, which trims
+ // all ASCII control bytes and spaces, including DELETE, from both ends.
+ // Unicode whitespace is intentionally preserved and fails to parse.
+ let untrimmed_time = if has_suffix {
+ &right_trimmed[..num_bytes - 2]
} else {
- trimmed
+ right_trimmed
};
+ let time_str = trim_all(untrimmed_time);
+ if time_str.is_empty() {
+ return None;
+ }
+
+ // parseTimestampString accepts a T-prefix only at the original segment
+ // start, before trimAll advances past any leading ASCII control bytes.
+ if time_str.starts_with('T') && !untrimmed_time.starts_with('T') {
+ return None;
+ }
// Parse the time components
let (hour, minute, second, micros) = parse_time_components(time_str)?;
@@ -479,4 +482,196 @@ mod tests {
assert_eq!(string_to_time(" T12:30:45"), None);
assert_eq!(string_to_time(" T12:30"), None);
}
+
+ #[test]
+ fn test_spark_trim_all_control_bytes() {
+ for (time, after_hour) in [("12:30:45", "30:45"), ("12:30", "30")] {
+ let expected = string_to_time(time);
+
+ for byte in (0_u8..=0x20).chain(std::iter::once(0x7f)) {
+ let padding = char::from(byte);
+ for input in [
+ format!("{padding}{time}"),
+ format!("{time}{padding}"),
+ format!("{padding}{time}{padding}"),
+ ] {
+ assert_eq!(
+ string_to_time(&input),
+ expected,
+ "padding byte 0x{byte:02X} in {input:?}"
+ );
+ }
+
+ let interior = format!("12:{padding}{after_hour}");
+ assert_eq!(
+ string_to_time(&interior),
+ None,
+ "interior padding byte 0x{byte:02X} in {interior:?}"
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_unicode_whitespace_is_not_trimmed() {
+ for padding in [
+ '\u{0085}', '\u{00a0}', '\u{1680}', '\u{2000}', '\u{2003}',
'\u{2007}', '\u{2028}',
+ '\u{2029}', '\u{202f}', '\u{205f}', '\u{3000}',
+ ] {
+ assert!(padding.is_whitespace());
+
+ for input in [
+ format!("{padding}12:30:45"),
+ format!("12:30:45{padding}"),
+ format!("{padding}12:30:45{padding}"),
+ format!("12:{padding}30:45"),
+ format!("{padding}1:00:00 AM"),
+ format!("1:00:00{padding}AM"),
+ format!("1:00:00 AM{padding}"),
+ ] {
+ assert_eq!(
+ string_to_time(&input),
+ None,
+ "Unicode whitespace U+{:04X} in {input:?}",
+ padding as u32
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_am_pm_control_byte_trimming() {
+ for (suffix, expected) in [
+ ("AM", NANOS_PER_HOUR),
+ ("PM", 13 * NANOS_PER_HOUR),
+ ("am", NANOS_PER_HOUR),
+ ("pm", 13 * NANOS_PER_HOUR),
+ ] {
+ for byte in (0_u8..=0x20).chain(std::iter::once(0x7f)) {
+ let padding = char::from(byte);
+
+ for input in [
+ format!("{padding}1:00:00 {suffix}"),
+ format!("1:00:00{padding}{suffix}"),
+ format!("{padding}1:00:00{padding}{suffix}"),
+ ] {
+ assert_eq!(
+ string_to_time(&input),
+ Some(expected),
+ "padding byte 0x{byte:02X} in {input:?}"
+ );
+ }
+
+ let trailing = format!("1:00:00 {suffix}{padding}");
+ let expected_trailing = (byte == b' ').then_some(expected);
+ assert_eq!(
+ string_to_time(&trailing),
+ expected_trailing,
+ "trailing padding byte 0x{byte:02X} in {trailing:?}"
+ );
+ }
+ }
+
+ assert_eq!(string_to_time("1:00:00 AM \t"), None);
+ assert_eq!(string_to_time("1:00:00 AM\t "), None);
+ assert_eq!(string_to_time("1:00:00 AM "), Some(NANOS_PER_HOUR));
+ }
+
+ #[test]
+ fn test_t_prefix_rejects_all_leading_trimmed_bytes() {
+ let expected = string_to_time("T12:30:45");
+
+ for byte in (0_u8..=0x20).chain(std::iter::once(0x7f)) {
+ let padding = char::from(byte);
+ let leading = format!("{padding}T12:30:45");
+ let trailing = format!("T12:30:45{padding}");
+
+ assert_eq!(
+ string_to_time(&leading),
+ None,
+ "leading padding byte 0x{byte:02X} in {leading:?}"
+ );
+ assert_eq!(
+ string_to_time(&trailing),
+ expected,
+ "trailing padding byte 0x{byte:02X} in {trailing:?}"
+ );
+ }
+ }
+
+ #[test]
+ fn test_t_prefix_am_pm_control_byte_trimming() {
+ for (time, seconds) in [("T1:30", 0), ("T1:30:45", 45)] {
+ for (suffix, hour) in [("AM", 1), ("PM", 13), ("am", 1), ("pm",
13)] {
+ let expected =
+ hour * NANOS_PER_HOUR + 30 * NANOS_PER_MINUTE + seconds *
NANOS_PER_SECOND;
+
+ assert_eq!(string_to_time(&format!("{time} {suffix}")),
Some(expected));
+
+ for byte in (0_u8..=0x20).chain(std::iter::once(0x7f)) {
+ let padding = char::from(byte);
+
+ for input in [
+ format!("{padding}{time} {suffix}"),
+ format!("{padding}{time}{padding}{suffix}"),
+ ] {
+ assert_eq!(
+ string_to_time(&input),
+ None,
+ "leading padding byte 0x{byte:02X} in {input:?}"
+ );
+ }
+
+ let before_suffix = format!("{time}{padding}{suffix}");
+ assert_eq!(
+ string_to_time(&before_suffix),
+ Some(expected),
+ "pre-suffix padding byte 0x{byte:02X} in
{before_suffix:?}"
+ );
+
+ let after_suffix = format!("{time} {suffix}{padding}");
+ assert_eq!(
+ string_to_time(&after_suffix),
+ (byte == b' ').then_some(expected),
+ "post-suffix padding byte 0x{byte:02X} in
{after_suffix:?}"
+ );
+ }
+ }
+ }
+ }
+
+ #[test]
+ fn test_spark_to_time_whitespace_error_modes() {
+ let input = StringArray::from(vec![
+ Some("\u{3000}12:30:45"),
+ Some("1:00:00 AM\t"),
+ Some("\u{1}12:30:45\u{7f}"),
+ Some("1:00:00\u{b}PM"),
+ None,
+ ]);
+ let args = [ColumnarValue::Array(Arc::new(input))];
+
+ assert!(matches!(
+ spark_to_time(&args, true),
+ Err(DataFusionError::Execution(message))
+ if message.contains("cannot be parsed to a TIME value")
+ ));
+
+ let ColumnarValue::Array(output) = spark_to_time(&args,
false).unwrap() else {
+ panic!("spark_to_time should return an array");
+ };
+ let output = output
+ .as_any()
+ .downcast_ref::<Time64NanosecondArray>()
+ .unwrap();
+
+ assert!(output.is_null(0));
+ assert!(output.is_null(1));
+ assert_eq!(
+ output.value(2),
+ 12 * NANOS_PER_HOUR + 30 * NANOS_PER_MINUTE + 45 * NANOS_PER_SECOND
+ );
+ assert_eq!(output.value(3), 13 * NANOS_PER_HOUR);
+ assert!(output.is_null(4));
+ }
}
diff --git
a/spark/src/test/resources/sql-tests/expressions/datetime/to_time.sql
b/spark/src/test/resources/sql-tests/expressions/datetime/to_time.sql
index b3ac439fd8..5522033f5c 100644
--- a/spark/src/test/resources/sql-tests/expressions/datetime/to_time.sql
+++ b/spark/src/test/resources/sql-tests/expressions/datetime/to_time.sql
@@ -242,6 +242,83 @@ SELECT try_to_time(' 12:30:45')
query
SELECT try_to_time(' 1:00:00 PM')
+-- Spark's time parser trims ASCII control characters, spaces, and DELETE
after detecting AM/PM,
+-- but never trims non-ASCII whitespace. Materialize the padding so every
query remains native.
+statement
+CREATE TABLE test_to_time_trim(name STRING, pad STRING) USING parquet
+
+statement
+INSERT INTO test_to_time_trim VALUES
+ ('a_none', ''),
+ ('b_nul_0x00', chr(0)),
+ ('c_soh_0x01', chr(1)),
+ ('d_tab_0x09', chr(9)),
+ ('e_vtab_0x0b', chr(11)),
+ ('f_us_0x1f', chr(31)),
+ ('g_space_0x20', ' '),
+ ('h_del_0x7f', chr(127)),
+ ('i_nbsp_u00a0', cast(X'C2A0' as string)),
+ ('j_ideographic_u3000', cast(X'E38080' as string))
+
+-- Leading, trailing, both-sided, and interior padding must match Spark for
every codepoint.
+query
+SELECT
+ name,
+ try_to_time(concat(pad, '12:30:45')),
+ try_to_time(concat('12:30:45', pad)),
+ try_to_time(concat(pad, '12:30:45', pad)),
+ try_to_time(concat('12:', pad, '30:45')),
+ try_to_time(concat(pad, '12:30')),
+ try_to_time(concat('12:30', pad)),
+ try_to_time(concat(pad, '12:30', pad)),
+ try_to_time(concat('12:', pad, '30'))
+FROM test_to_time_trim
+
+-- Leading trimAll padding invalidates a T prefix. Controls before AM/PM are
valid, but only ASCII
+-- spaces after AM/PM are trimmed before Spark checks the suffix, including
with a T prefix.
+query
+SELECT
+ name,
+ try_to_time(concat(pad, 'T12:30:45')),
+ try_to_time(concat('T12:30:45', pad)),
+ try_to_time(concat('1:00:00', pad, 'PM')),
+ try_to_time(concat('1:00:00 PM', pad)),
+ try_to_time(concat(pad, 'T12:30:45 PM')),
+ try_to_time(concat('T12:30:45', pad, 'PM')),
+ try_to_time(concat('T12:30:45 PM', pad)),
+ try_to_time(concat(pad, 'T12:30 PM')),
+ try_to_time(concat('T12:30', pad, 'PM')),
+ try_to_time(concat('T12:30 PM', pad))
+FROM test_to_time_trim
+
+-- The throwing variant must accept the same valid ASCII controls as
try_to_time.
+query
+SELECT
+ name,
+ to_time(concat(pad, '12:30:45', pad)),
+ to_time(concat('1:00:00', pad, 'PM'))
+FROM test_to_time_trim
+WHERE name IN (
+ 'a_none', 'b_nul_0x00', 'c_soh_0x01', 'd_tab_0x09', 'e_vtab_0x0b',
+ 'f_us_0x1f', 'g_space_0x20', 'h_del_0x7f')
+
+-- The throwing variant must reject Unicode whitespace instead of silently
parsing it.
+query expect_error(cannot be parsed to a TIME value)
+SELECT to_time(concat(pad, '12:30:45'))
+FROM test_to_time_trim
+WHERE name = 'i_nbsp_u00a0'
+
+query expect_error(cannot be parsed to a TIME value)
+SELECT to_time(concat('12:30:45', pad))
+FROM test_to_time_trim
+WHERE name = 'j_ideographic_u3000'
+
+-- Only literal ASCII spaces are removed before AM/PM suffix detection.
+query expect_error(cannot be parsed to a TIME value)
+SELECT to_time(concat('1:00:00 PM', pad))
+FROM test_to_time_trim
+WHERE name = 'd_tab_0x09'
+
-- to_time with format pattern falls back to Spark (not supported natively)
query expect_fallback(invoke is not supported)
SELECT to_time('12:30:45', 'HH:mm:ss')
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
