mbutrovich commented on code in PR #4914:
URL: https://github.com/apache/datafusion-comet/pull/4914#discussion_r3591069919
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -254,6 +254,28 @@ where
pruned_float_str.parse::<F>().ok()
}
+/// Parse the boolean literals Spark accepts in a string-to-boolean cast,
returning
+/// `None` for anything else.
+///
+/// Trimming before comparing is equivalent to trimming afterwards: ASCII case
folding
+/// never turns a whitespace character into a non-whitespace one, or vice
versa.
+#[inline]
+fn parse_utf8_to_boolean(value: &str) -> Option<bool> {
+ let trimmed = value.trim();
Review Comment:
`string.rs:264` uses Rust `str::trim` (Unicode `char::is_whitespace`).
Spark's `UTF8String.trimAll()` (`UTF8String.java:994-1010`) trims every byte
where `Character.isWhitespace(b) || Character.isISOControl(b)`
(`UTF8String.java:201-203`). These sets differ on the ASCII control bytes
`0x00-0x08`, `0x0E-0x1F`, and `0x7F`: Spark trims them, Rust does not. I
confirmed the full set with a byte-by-byte comparison of `char::is_whitespace`
against Java's `isWhitespace || isISOControl`.
Concretely, `" \x01true\x01 "` casts to `true` in Spark but to `null` (or
ANSI-errors) in Comet. This is pre-existing: the old
`.to_ascii_lowercase().trim()` had the same behavior, so this PR is not a
regression. It is out of scope to fix here, but the PR adds a fresh
`test_parse_utf8_to_boolean` that tests whitespace trimming (`string.rs`, `"
\ttrue\n "`) without touching control chars, so it silently locks in the
divergence.
The existing integration test at `CometCastSuite.scala:806-811` uses
`whitespaceChars = " \t\r\n"` (`CometCastSuite.scala:61`), all of which agree
between Rust and Spark, so nothing catches the control-char gap.
Suggested change: leave the trimming as-is for this PR to keep it a pure
perf change, and file a follow-up issue for `trimAll` parity. Do not add a
passing assertion like `assert_eq!(parse_utf8_to_boolean("\x01true\x01"),
None)` to the new test, since that would enshrine incorrect behavior as
expected. If you want a marker, add a `// TODO(#issue): Spark trimAll also
strips ISO control bytes` comment near the `value.trim()` call.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1989,6 +2015,38 @@ mod tests {
}
}
+ #[test]
+ fn test_parse_utf8_to_boolean() {
Review Comment:
`test_parse_utf8_to_boolean` (`string.rs`, invalid list) exercises `"úñî"`
(6 bytes, falls to the `_` arm) and `"ｔｒｕｅ"` (fullwidth, 15 bytes, `_` arm).
Neither hits a 2-to-5 byte arm where `eq_ignore_ascii_case` does the real work
against a non-ASCII input of the exact target length. That path is the one most
likely to regress if someone later swaps the comparison for something
byte-naive.
Suggested change: add a 2-byte non-ASCII value that must not match `"no"`,
for example `assert_eq!(parse_utf8_to_boolean("ñ"), None)` (`"ñ"` is 2 bytes),
and a 4-byte one for the `"true"` arm such as
`assert_eq!(parse_utf8_to_boolean("a\u{0300}bc"), None)` or simply
`"tру"`-style mixed bytes summing to 4.
--
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]
