sunchao commented on code in PR #5364:
URL: https://github.com/apache/datafusion-comet/pull/5364#discussion_r3789854300
##########
native/spark-expr/src/datetime_funcs/to_time.rs:
##########
@@ -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();
Review Comment:
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.
--
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]
