You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by su...@apache.org on 2026/08/15 03:30:16 UTC
(datafusion-comet) 01/01: fix: match Spark whitespace trimming in to_time and try_to_time
This is an automated email from the ASF dual-hosted git repository.
sunchao pushed a commit to branch dev/chao/codex/fix-to-time-trim-semantics
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
commit b4b3aa85f53d76e89433f660bd13c63fd11db313
Author: Chao Sun <ch...@openai.com>
AuthorDate: Fri Aug 14 19:57:47 2026 -0700
fix: match Spark whitespace trimming in to_time and try_to_time
---
.../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 | 190 ++++++++++++++++++---
.../sql-tests/expressions/datetime/to_time.sql | 67 ++++++++
5 files changed, 247 insertions(+), 26 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..fe7a4dd951 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,19 +81,11 @@ 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();
+ // 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_chars = bytes.len();
// Detect AM/PM suffix
@@ -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_chars - 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,153 @@ 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() {
+ let expected = string_to_time("12:30:45");
+
+ for byte in (0_u8..=0x20).chain(std::iter::once(0x7f)) {
+ let padding = char::from(byte);
+ for input in [
+ format!("{padding}12:30:45"),
+ format!("12:30:45{padding}"),
+ format!("{padding}12:30:45{padding}"),
+ ] {
+ assert_eq!(
+ string_to_time(&input),
+ expected,
+ "padding byte 0x{byte:02X} in {input:?}"
+ );
+ }
+
+ let interior = format!("12:{padding}30:45");
+ 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_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..210790cda6 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,73 @@ 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'))
+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.
+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))
+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: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org