mbutrovich commented on code in PR #26377:
URL: https://github.com/apache/datafusion/pull/26377#discussion_r4285968059
##########
datafusion/functions/src/string/common.rs:
##########
@@ -390,38 +390,48 @@ fn unicode_case(s: &str, lower: bool) -> String {
}
}
-/// Like [`unicode_case`], but uppercases into `buf` so its allocation is
reused
-/// across rows. Lowercasing still allocates, see [`write_unicode_case`].
+/// Appends `s` to `builder` with its case converted.
+///
+/// An ASCII value is converted byte by byte straight into the builder. Other
+/// values go through `str::to_lowercase` / `str::to_uppercase`, which allocate
+/// a `String` but convert any leading run of ASCII bytes in bulk.
#[inline]
-fn unicode_case_into<'a>(s: &str, lower: bool, buf: &'a mut String) -> &'a str
{
- if lower {
- *buf = s.to_lowercase();
- } else {
- buf.clear();
- buf.extend(s.chars().flat_map(char::to_uppercase));
+fn append_case_converted<O: OffsetSizeTrait>(
Review Comment:
> I agree this makes sense but I'd prefer to defer to a followup PR.
Could you open an issue for the follow-up and link it here? It also matters
for `translate`.
[`append_translated_ascii`](https://github.com/apache/datafusion/blob/c52eb9734b4c968ad9880fb82f3214efd88cd82b/datafusion/functions/src/unicode/translate.rs#L351)
goes through
[`BulkNullStringArrayBuilder::append_byte_map`](https://github.com/apache/datafusion/blob/b5bb8f23444a081ac5aa3f6dd17eeb5cf8e75c41/datafusion/functions/src/strings.rs#L1158),
which only has a panicking form. So after this PR, `upper` and `lower` return
an error on offset overflow but `translate` still panics. Adding
`try_append_byte_map` to the trait would fix both.
--
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]
