andygrove commented on code in PR #26377:
URL: https://github.com/apache/datafusion/pull/26377#discussion_r4285097822
##########
datafusion/functions/src/string/lower.rs:
##########
@@ -142,27 +142,6 @@ mod tests {
Ok(())
}
- #[test]
- fn lower_maybe_optimization() -> Result<()> {
- let input = Arc::new(StringArray::from(vec![
- Some("农历新年"),
- None,
- Some("DATAFUSION"),
- Some("0123456789"),
- Some(""),
- ])) as ArrayRef;
-
- let expected = Arc::new(StringArray::from(vec![
- Some("农历新年"),
- None,
- Some("datafusion"),
- Some("0123456789"),
- Some(""),
- ])) as ArrayRef;
-
- to_lower(input, expected)
- }
-
#[test]
Review Comment:
The removed `lower_partial_optimization` was the only test that put
non-final and final Greek sigma in the same non-ASCII row (`'ὈΔΥΣΣΕΎΣ'` ->
`'ὀδυσσεύς'`, which needs σ mid-word and ς at the end). The new
`lower_mixed_test` only has `'ABΣ'` and `'...ΟΔΟΣ'`, which are both final-sigma
cases, so a change to `append_case_converted` that broke context-sensitive
sigma handling (e.g. a per-char fallback for non-ASCII rows) would still pass.
Could we keep a case like that, plus the non-ASCII Utf8View cases that were
removed?
##########
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>(
+ builder: &mut GenericStringArrayBuilder<O>,
+ s: &str,
+ lower: bool,
+) -> Result<()> {
Review Comment:
Every non-ASCII row scans the string twice (`is_ascii()` then conversion)
and allocates a fresh `String` via `to_lowercase()`/`to_uppercase()`. That
looks like why the gain on 'Non-ASCII words' is only ~4-12% and 'Late
non-ASCII' lower is 7-8% slower. It's a reasonable trade-off, but it might be
worth a comment documenting it.
##########
datafusion/functions/src/strings.rs:
##########
@@ -829,38 +832,78 @@ impl StringViewArrayBuilder {
Self::make_long_view_checked(length, buffer_index, offset,
prefix_bytes)
}
- /// See [`BulkNullStringArrayBuilder::append_byte_map`].
+ /// Fallible variant of [`Self::append_byte_map`].
///
/// # Safety
///
/// The bytes produced by applying `map` to each byte of `src`, in order,
/// must form valid UTF-8.
///
- /// # Panics
+ /// # Errors
///
- /// Panics under the same conditions as [`Self::append_value`]: if
- /// `src.len()`, the in-progress buffer offset, or the number of completed
- /// buffers exceeds `i32::MAX`.
+ /// Returns an error under the same conditions as
+ /// [`Self::try_append_value`].
#[inline]
- pub unsafe fn append_byte_map<F: FnMut(u8) -> u8>(&mut self, src: &[u8],
mut map: F) {
- let length: u32 =
- i32::try_from(src.len()).expect("value length exceeds i32::MAX")
as u32;
- if length <= 12 {
- let mut bytes = [0u8; 12];
- for (d, &b) in bytes[..src.len()].iter_mut().zip(src) {
+ pub unsafe fn try_append_byte_map<F: FnMut(u8) -> u8>(
+ &mut self,
+ src: &[u8],
+ mut map: F,
+ ) -> Result<()> {
+ if src.len() <= MAX_INLINE_LEN {
+ // An inline view holds the length in its low 4 bytes, followed by
+ // the value zero-padded to 12 bytes. Writing the mapped bytes
+ // straight into it is faster than going through `inline_view`.
+ let mut view = (src.len() as u128).to_le_bytes();
+ for (d, &b) in view[4..].iter_mut().zip(src) {
*d = map(b);
}
- self.views.push(make_view(&bytes[..src.len()], 0, 0));
- return;
+ self.views.push(u128::from_le_bytes(view));
+ return Ok(());
}
- self.ensure_long_capacity(length);
+ let length: u32 = i32::try_from(src.len())
+ .map_err(|_| string_view_overflow_error("value length"))?
+ as u32;
+ self.try_ensure_long_capacity(length)?;
let cursor = self.in_progress.len();
- let offset: u32 = i32::try_from(cursor).expect("offset exceeds
i32::MAX") as u32;
+ let offset: u32 = i32::try_from(cursor)
+ .map_err(|_| string_view_overflow_error("offset"))?
+ as u32;
+ let buffer_index: u32 = i32::try_from(self.completed.len())
+ .map_err(|_| string_view_overflow_error("buffer count"))?
+ as u32;
self.in_progress.extend(src.iter().map(|&b| map(b)));
Review Comment:
The long-string branch of the view builder still maps one byte at a time
with `extend(src.iter().map(..))`. The generic builder was changed to
copy-then-map in place because the iterator form stayed scalar. The same change
would likely help here for Utf8View values longer than 12 bytes (e.g. the
`Utf8View, len 128` ASCII rows).
##########
datafusion/functions/benches/string_expressions/case_conversion_inputs.rs:
##########
@@ -0,0 +1,198 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Inputs shared by the `upper` and `lower` benchmarks, so both functions are
+//! measured on the same arrays.
+
+use std::hint::black_box;
+use std::sync::Arc;
+
+use crate::helper::gen_string_array;
+use arrow::array::{Array, ArrayRef, LargeStringArray, StringArray,
StringViewArray};
+use arrow::datatypes::Field;
+use arrow::util::bench_util::{
+ create_string_array_with_len, create_string_view_array_with_len,
+};
+use criterion::Criterion;
+use datafusion_common::config::ConfigOptions;
+use datafusion_expr::{ColumnarValue, ScalarFunctionArgs, ScalarUDF};
+
+/// Rows in each array built by [`build_values`].
+const SIZE: usize = 4096;
+
+const WORDS: [&str; 8] = [
+ "café",
+ "straße",
+ "αλφα",
+ "こんにちは",
+ "münchen",
+ "naïve",
+ " órdenes",
+ "tschüß",
+];
+
+/// Builds [`SIZE`] values: every tenth is NULL and the rest come from `value`.
+fn build_values(value: impl Fn(usize) -> String) -> Vec<Option<String>> {
+ (0..SIZE).map(|i| (i % 10 != 0).then(|| value(i))).collect()
+}
+
+/// Three words from [`WORDS`], so the value is non-ASCII.
+fn word_value(row: usize) -> String {
+ let a = WORDS[row % WORDS.len()];
+ let b = WORDS[(row * 7 + 3) % WORDS.len()];
+ format!("{a} {b} {a}")
+}
+
+/// ASCII text of exactly `len` bytes that differs from row to row.
+fn ascii_value(row: usize, len: usize) -> String {
+ let base = format!("datafusion row {row} ");
+ base.repeat(len / base.len() + 1)[..len].to_string()
+}
+
+/// Returns the named input arrays.
+///
+/// Entirely ASCII, which take the all-ASCII array fast path:
+///
+/// - `all_ascii_utf8_rows*`: 32-byte values, 20% NULL.
+/// - `all_ascii_utf8_sliced`: 128 rows sliced from the middle of a larger
+/// array, so the input's buffers are much larger than the rows it uses.
+/// - `all_ascii_utf8view_*`: values of `len` bytes, or with `_mixed`, half
+/// under 12 bytes (stored inline in the view) and half of 12 to
+/// `max(30, len)` bytes.
+///
+/// Not entirely ASCII, so each row is converted on its own:
+///
+/// - `some_non_ascii_utf8view_*`: 30-character values, about 10% of them
+/// non-ASCII.
+/// - `non_ascii_words_*`: every value is three words from [`WORDS`].
+/// - `mostly_ascii_*`: ASCII values plus one non-ASCII value in the middle.
+/// - `late_non_ascii_*`: every value is ASCII text followed by one non-ASCII
+/// character.
+pub fn inputs() -> Vec<(String, ArrayRef)> {
+ let mut inputs: Vec<(String, ArrayRef)> = Vec::new();
+
Review Comment:
`inputs()` builds ~45 arrays up front (including 65536-row and 8192-row
ones) and is called separately by the `upper` and `lower` benchmarks, so
running the whole `string_expressions` bench or filtering to a single name
still builds everything, twice. Building lazily or sharing the inputs would
help. Also, since `misc/upper.rs` and `upper_unicode.rs` were removed and
`lower.rs` rewritten, historical benchmark comparisons by name no longer line
up.
##########
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:
`append_case_converted` and `append_case_converted_view` (here and
`append_case_converted_view` at line 420) are identical copies that differ only
in the builder type: the `is_ascii` check, the `unicode_case` fallback and the
unsafe `try_append_byte_map` dispatch are written twice. One option is to add a
fallible `try_append_byte_map` to the `BulkNullStringArrayBuilder` trait (today
it only has the panicking `append_byte_map`) and make this a single generic
function.
##########
datafusion/functions/src/strings.rs:
##########
@@ -829,38 +832,78 @@ impl StringViewArrayBuilder {
Self::make_long_view_checked(length, buffer_index, offset,
prefix_bytes)
}
- /// See [`BulkNullStringArrayBuilder::append_byte_map`].
+ /// Fallible variant of [`Self::append_byte_map`].
///
/// # Safety
///
/// The bytes produced by applying `map` to each byte of `src`, in order,
/// must form valid UTF-8.
///
- /// # Panics
+ /// # Errors
///
- /// Panics under the same conditions as [`Self::append_value`]: if
- /// `src.len()`, the in-progress buffer offset, or the number of completed
- /// buffers exceeds `i32::MAX`.
+ /// Returns an error under the same conditions as
+ /// [`Self::try_append_value`].
#[inline]
- pub unsafe fn append_byte_map<F: FnMut(u8) -> u8>(&mut self, src: &[u8],
mut map: F) {
- let length: u32 =
- i32::try_from(src.len()).expect("value length exceeds i32::MAX")
as u32;
- if length <= 12 {
- let mut bytes = [0u8; 12];
- for (d, &b) in bytes[..src.len()].iter_mut().zip(src) {
+ pub unsafe fn try_append_byte_map<F: FnMut(u8) -> u8>(
Review Comment:
The error-vs-panic behaviour is the stated reason for adding
`try_append_byte_map`, but the new overflow error paths have no tests. The
success path test was extended, but nothing exercises the value-length, offset
or buffer-count errors, or Utf8/LargeUtf8 offset overflow. A regression back to
`expect`/panic in `upper`/`lower`, `translate` or `replace` would go unnoticed.
--
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]
