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