fix: match Spark whitespace trimming in to_time and try_to_time

[sunchao]

Which issue does this PR close?

Part of #5149. This PR fixes to_time and try_to_time; the umbrella issue remains open for the outstanding timestamp casts.

Why are the changes needed?

Enabling Comet should not change the result of a Spark expression or turn a failing query into a successful one. For the Spark 4.1 TIME functions, however, the native parser currently disagrees with Spark about which characters around a time value count as whitespace.

Comet uses Rust's Unicode-aware str::trim(). Spark instead accepts a specific set of ASCII control characters while rejecting non-ASCII whitespace. Those definitions differ in both directions, so the same query can either lose a valid value or silently manufacture an invalid one:

-- An ASCII control character (SOH, 0x01) is valid padding in Spark.
SELECT try_to_time(concat(chr(1), '12:30:45'));
-- Spark:             12:30:45
-- Comet before:      NULL
-- Comet after:       12:30:45

-- An ideographic space (U+3000) is not valid padding in Spark.
SELECT try_to_time(concat(cast(X'E38080' AS STRING), '12:30:45'));
-- Spark:             NULL
-- Comet before:      12:30:45
-- Comet after:       NULL

-- A tab after an AM/PM suffix also makes the input invalid.
SELECT try_to_time(concat('1:00:00 PM', chr(9)));
-- Spark:             NULL
-- Comet before:      13:00:00
-- Comet after:       NULL

The second and third cases are silent correctness bugs: try_to_time returns a real value where Spark returns NULL. For to_time, the consequence is stronger: Comet successfully returns a value for input on which Spark raises a parsing error.

The AM/PM case also explains why replacing one generic trim call with another is not sufficient. Spark treats the position of the control character as significant:

SELECT try_to_time(concat('1:00:00', chr(9), 'PM'));  -- 13:00:00
SELECT try_to_time(concat('1:00:00 PM', chr(9)));     -- NULL

A tab before PM belongs to the time portion and can be trimmed. A tab after PM prevents Spark from recognizing the suffix, so the entire input is invalid.

What changes were proposed in this PR?

The native parser now follows the same two-stage parsing model as Spark.

First, it removes only literal ASCII spaces from the right side of the original input and looks for an optional AM or PM suffix. Once that suffix has been identified, the parser trims the remaining time portion with the existing Spark-compatible trim_all helper. That helper removes exactly the ASCII bytes 0x00–0x20 and 0x7F, while leaving Unicode whitespace untouched.

Keeping those two stages separate reproduces the otherwise surprising distinction between controls before and after an AM/PM suffix. Reusing the shared helper also keeps time parsing consistent with the other native conversions that already implement Spark's trimming rules. The parser continues to enforce Spark's original-start requirement for a T prefix and preserves the existing difference between to_time, which reports invalid input, and try_to_time, which returns NULL.

The new regression coverage exercises these behaviors in both the native parser and real Spark SQL execution. Compatibility documentation is updated only for the two time functions fixed here; the separate timestamp-cast discrepancies remain documented and tracked by #5149.

How was this PR tested?

Native tests cover all 34 ASCII/control bytes trimmed by Spark, representative Unicode whitespace, leading and trailing padding, AM/PM placement, T prefixes, nulls, and both error modes. The SQL tests materialize the inputs in Parquet so the expressions run through Comet's native operator instead of being folded into constants.

cargo fmt --all -- --check
cargo test -p datafusion-comet-spark-expr --lib
cargo clippy -p datafusion-comet-spark-expr --tests -- -D warnings

./mvnw --no-transfer-progress -Pspark-4.1 test \
  -Dtest=none \
  '-Dsuites=org.apache.comet.CometSqlFileTestSuite sql-file: expressions/datetime/to_time.sql'

All 632 Rust tests passed, and the focused Spark 4.1.3 integration suite passed, including the expected native parsing errors. Formatting and Clippy checks also passed. An additional differential comparison against Spark covered 136,584 inputs without finding a newly introduced production mismatch.

[andygrove]

Thanks for this, the two-stage split is the right model and the write-up made it easy to follow.

I checked the fix by porting Spark 4.1's stringToTime and parseTimestampString to Rust straight from source and running it against both the pre-PR and post-PR versions of string_to_time over ~107k inputs: 30 core time strings crossed with all 34 trimAll bytes and seven Unicode whitespace codepoints, applied as leading, trailing, doubled, interior and pre-suffix padding. Mismatches drop from 24,709 across 212 distinct shapes to 7,494 across 30 shapes, and not one whitespace-shaped mismatch survives. The T-prefix guard rewrite lines up exactly with Spark's j == 0 && b == 'T' gate too.

I also confirmed the reference implementation is the right one. ToTimeParser with no format uses TimeFormatter(None, isParsing = true), which is DefaultTimeFormatter, whose parse calls SparkDateTimeUtils.stringToTimeAnsi.

The 30 surviving mismatch shapes are all pre-existing and unrelated to whitespace. They reduce to three root causes, and two of them are the mirror image of what this PR fixes, where Comet raises on input Spark accepts (to_time('T12') and to_time('12:30:45.')). I filed them separately as #5366 so they do not get lost. Nothing owed on this PR for those.

Approving. The two comments below are things I would like addressed first.

[andygrove]

Could this be renamed to num_bytes? The name is carried over from Spark, but Spark's numChars is a codepoint count and getChar indexes by codepoint, whereas this is bytes.len().

The two do agree here, and I worked out why: UTF-8 continuation bytes are all >= 0x80, so the last two bytes can only read as A/a/P/p followed by M/m when they genuinely are two ASCII codepoints, and in that case numChars > 2 and numBytes > 2 are equivalent. But that reasoning is not written down anywhere. This is the one file where the next person will be diffing against Spark line by line, and a byte count named num_chars is exactly the kind of thing that will stop them. A rename plus a one-line comment on why the byte-based check is safe would settle it.

[sunchao]

Renamed it to num_bytes and added a comment explaining why the byte-based suffix check is safe: ASCII AM/PM bytes cannot be UTF-8 continuation bytes.

[andygrove]

This coverage is thorough, and asserting that the trailing-after-suffix case is None for every byte except 0x20 is exactly the right shape.

One combination is missing from both here and the SQL fixture: the T prefix together with an AM/PM suffix, say T12:30:45 PM and a control-byte-prefixed variant. That is the one path where both trim stages apply, and it is the interaction the PR description leads with, so it seems worth pinning. I checked and it behaves correctly today, so this is only about locking it in against a future refactor that fixes one stage and breaks the other.

The HH:mm form without seconds is also not in the new padding matrix, which only uses 12:30:45 and 1:00:00.

[sunchao]

Added Rust and Parquet-backed SQL coverage for T + AM/PM combinations and padded HH:mm inputs. The Rust tests exercise all 34 trim bytes before T, before the suffix, and after the suffix across both HH:mm/HH:mm:ss forms and upper/lowercase suffixes. The full native suite (633 tests) and the Spark 4.1 SQL fixture both pass.