You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "theirix (via GitHub)" <gi...@apache.org> on 2026/01/29 22:57:53 UTC
[PR] perf: optimise right for byte access and StringView [datafusion]
theirix opened a new pull request, #20069:
URL: https://github.com/apache/datafusion/pull/20069
## Which issue does this PR close?
- Closes #20068.
## Rationale for this change
Similar to issue #19749 and the optimisation of `left` in #19980, it's worth doing the same for `right`
## What changes are included in this PR?
- Improve efficiency of the function by making fewer memory allocations and going directly to bytes, based on char boundaries
- Provide a specialisation for StringView with buffer zero-copy
- Use `arrow_array::buffer::make_view` for low-level view manipulation (we still need to know about a magic constant 12 for a buffer layout)
- Benchmark - up to 90% performance improvement
```
right size=1024/string_array positive n/1024
time: [24.286 µs 24.658 µs 25.087 µs]
change: [−86.881% −86.662% −86.424%] (p = 0.00 < 0.05)
Performance has improved.
right size=1024/string_array negative n/1024
time: [29.996 µs 30.737 µs 31.511 µs]
change: [−89.442% −89.229% −89.003%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
right size=4096/string_array positive n/4096
time: [105.58 µs 109.39 µs 113.51 µs]
change: [−86.119% −85.788% −85.497%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
6 (6.00%) high mild
3 (3.00%) high severe
right size=4096/string_array negative n/4096
time: [136.48 µs 138.34 µs 140.36 µs]
change: [−88.007% −87.848% −87.692%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild
right size=1024/string_view_array positive n/1024
time: [25.054 µs 25.500 µs 26.033 µs]
change: [−82.569% −82.285% −81.891%] (p = 0.00 < 0.05)
Performance has improved.
right size=1024/string_view_array negative n/1024
time: [41.281 µs 42.730 µs 44.432 µs]
change: [−73.832% −73.288% −72.716%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe
right size=4096/string_view_array positive n/4096
time: [129.38 µs 133.69 µs 137.61 µs]
change: [−79.497% −78.998% −78.581%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild
right size=4096/string_view_array negative n/4096
time: [218.16 µs 229.41 µs 243.30 µs]
change: [−65.405% −63.622% −61.515%] (p = 0.00 < 0.05)
Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
3 (3.00%) high mild
7 (7.00%) high severe
```
## Are these changes tested?
- Existing unit tests for `right`
- Added more unit tests
- Added bench similar to `right.rs`
- Existing SLTs pass
## Are there any user-facing changes?
No
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#discussion_r2746466660
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,140 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // println!(
+ // "Input string: {}, n: {} -> byte_length: {} -> {}",
+ // &string,
+ // n,
+ // byte_length,
+ // &string[byte_length..]
+ // );
+ // Extract starting from `byte_length` bytes from a byte-indexed slice
+ Some(&string[byte_length..])
+ }
_ => None,
})
.collect::<GenericStringArray<T>>();
Ok(Arc::new(result) as ArrayRef)
}
+/// `right` implementation for StringViewArray
+fn right_impl_view(
+ string_view_array: &StringViewArray,
+ n_array: &Int64Array,
+) -> Result<ArrayRef> {
+ let len = n_array.len();
+
+ let views = string_view_array.views();
+ // Every string in StringViewArray has one corresponding view in `views`
+ debug_assert!(views.len() == string_view_array.len());
+
+ // Compose null buffer at once
+ let string_nulls = string_view_array.nulls();
+ let n_nulls = n_array.nulls();
+ let new_nulls = NullBuffer::union(string_nulls, n_nulls);
+
+ let new_views = (0..len)
+ .map(|idx| {
+ let view = views[idx];
+
+ let is_valid = match &new_nulls {
+ Some(nulls_buf) => nulls_buf.is_valid(idx),
+ None => true,
+ };
+
+ if is_valid {
+ let string: &str = string_view_array.value(idx);
+ let n = n_array.value(idx);
+
+ let new_offset = right_byte_length(string, n);
+ let result_bytes = &string.as_bytes()[new_offset..];
+
+ if result_bytes.len() > 12 {
+ let byte_view = ByteView::from(view);
+ // Reuse buffer, but adjust offset and length
+ make_view(
+ result_bytes,
+ byte_view.buffer_index,
+ byte_view.offset + new_offset as u32,
+ )
+ } else {
+ // inline value does not need block id or offset
+ make_view(result_bytes, 0, 0)
+ }
+ } else {
+ // For nulls, keep the original view
+ view
+ }
+ })
+ .collect::<Vec<u128>>();
+
+ // Buffers are unchanged
+ let result = StringViewArray::try_new(
+ ScalarBuffer::from(new_views),
+ Vec::from(string_view_array.data_buffers()),
+ new_nulls,
+ )?;
+ Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Calculate the byte length of the substring of last `n` chars from string `string`
+/// (or all but first `|n|` chars if n is negative)
+fn right_byte_length(string: &str, n: i64) -> usize {
+ match n.cmp(&0) {
+ Ordering::Less => string
+ .char_indices()
+ .nth(n.unsigned_abs() as usize)
+ .map(|(index, _)| index)
+ .unwrap_or(string.len()),
+ Ordering::Equal => string.len(),
+ Ordering::Greater => string
+ .char_indices()
+ .nth_back(n.unsigned_abs() as usize - 1)
Review Comment:
nit: This may truncate on 32-bit machines
##########
datafusion/functions/benches/right.rs:
##########
@@ -0,0 +1,140 @@
+// 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.
+
+extern crate criterion;
+
+use std::hint::black_box;
+use std::sync::Arc;
+
+use arrow::array::{ArrayRef, Int64Array};
+use arrow::datatypes::{DataType, Field};
+use arrow::util::bench_util::{
+ create_string_array_with_len, create_string_view_array_with_len,
+};
+use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main};
+use datafusion_common::config::ConfigOptions;
+use datafusion_expr::{ColumnarValue, ScalarFunctionArgs};
+use datafusion_functions::unicode::right;
+
+fn create_args(
+ size: usize,
+ str_len: usize,
+ use_negative: bool,
+ is_string_view: bool,
+) -> Vec<ColumnarValue> {
+ let string_arg = if is_string_view {
+ ColumnarValue::Array(Arc::new(create_string_view_array_with_len(
+ size, 0.1, str_len, true,
+ )))
+ } else {
+ ColumnarValue::Array(Arc::new(create_string_array_with_len::<i32>(
+ size, 0.1, str_len,
+ )))
+ };
+
+ // For negative n, we want to trigger the double-iteration code path
+ let n_values: Vec<i64> = if use_negative {
+ (0..size).map(|i| -((i % 10 + 1) as i64)).collect()
+ } else {
+ (0..size).map(|i| (i % 10 + 1) as i64).collect()
+ };
+ let n_array = Arc::new(Int64Array::from(n_values));
+
+ vec![
+ string_arg,
+ ColumnarValue::Array(Arc::clone(&n_array) as ArrayRef),
+ ]
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+ for is_string_view in [false, true] {
+ for size in [1024, 4096] {
+ let mut group = c.benchmark_group(format!("right size={size}"));
+
+ // Benchmark with positive n (no optimization needed)
+ let mut function_name = if is_string_view {
+ "string_view_array positive n"
+ } else {
+ "string_array positive n"
+ };
+ let args = create_args(size, 32, false, is_string_view);
+ group.bench_function(BenchmarkId::new(function_name, size), |b| {
+ let arg_fields = args
+ .iter()
+ .enumerate()
+ .map(|(idx, arg)| {
+ Field::new(format!("arg_{idx}"), arg.data_type(), true).into()
+ })
+ .collect::<Vec<_>>();
+ let config_options = Arc::new(ConfigOptions::default());
+
+ b.iter(|| {
+ black_box(
+ right()
+ .invoke_with_args(ScalarFunctionArgs {
+ args: args.clone(),
+ arg_fields: arg_fields.clone(),
+ number_rows: size,
+ return_field: Field::new("f", DataType::Utf8, true)
Review Comment:
Shouldn't this use `Utf8View` when `is_string_view == true` ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#discussion_r2744053111
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,140 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // println!(
+ // "Input string: {}, n: {} -> byte_length: {} -> {}",
+ // &string,
+ // n,
+ // byte_length,
+ // &string[byte_length..]
+ // );
+ // Extract starting from `byte_length` bytes from a byte-indexed slice
+ Some(&string[byte_length..])
+ }
_ => None,
})
.collect::<GenericStringArray<T>>();
Ok(Arc::new(result) as ArrayRef)
}
+/// `right` implementation for StringViewArray
+fn right_impl_view(
+ string_view_array: &StringViewArray,
+ n_array: &Int64Array,
+) -> Result<ArrayRef> {
+ let len = n_array.len();
+
+ let views = string_view_array.views();
+ // Every string in StringViewArray has one corresponding view in `views`
+ debug_assert!(views.len() == string_view_array.len());
+
+ // Compose null buffer at once
+ let string_nulls = string_view_array.nulls();
+ let n_nulls = n_array.nulls();
+ let new_nulls = NullBuffer::union(string_nulls, n_nulls);
+
+ let new_views = (0..len)
+ .map(|idx| {
+ let view = views[idx];
+
+ let is_valid = match &new_nulls {
+ Some(nulls_buf) => nulls_buf.is_valid(idx),
+ None => true,
+ };
+
+ if is_valid {
+ let string: &str = string_view_array.value(idx);
+ let n = n_array.value(idx);
+
+ let new_offset = right_byte_length(string, n);
+ let result_bytes = &string.as_bytes()[new_offset..];
+
+ if result_bytes.len() > 12 {
+ let byte_view = ByteView::from(view);
+ // Reuse buffer, but adjust offset and length
+ make_view(
+ result_bytes,
+ byte_view.buffer_index,
+ byte_view.offset + new_offset as u32,
+ )
+ } else {
+ // inline value does not need block id or offset
+ make_view(result_bytes, 0, 0)
+ }
+ } else {
+ // For nulls, keep the original view
+ view
+ }
+ })
+ .collect::<Vec<u128>>();
+
+ // Buffers are unchanged
+ let result = StringViewArray::try_new(
+ ScalarBuffer::from(new_views),
+ Vec::from(string_view_array.data_buffers()),
+ new_nulls,
+ )?;
+ Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Calculate the byte length of the substring of last `n` chars from string `string`
+/// (or all but first `|n|` chars if n is negative)
+fn right_byte_length(string: &str, n: i64) -> usize {
Review Comment:
I haven't looked too closely, but I feel we can deduplicate right + left implementation code as the main difference is this byte length function? In that it flips which side it looks from?
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,140 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // println!(
Review Comment:
Commented code accidentally added here
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#discussion_r2749774658
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,140 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // println!(
+ // "Input string: {}, n: {} -> byte_length: {} -> {}",
+ // &string,
+ // n,
+ // byte_length,
+ // &string[byte_length..]
+ // );
+ // Extract starting from `byte_length` bytes from a byte-indexed slice
+ Some(&string[byte_length..])
+ }
_ => None,
})
.collect::<GenericStringArray<T>>();
Ok(Arc::new(result) as ArrayRef)
}
+/// `right` implementation for StringViewArray
+fn right_impl_view(
+ string_view_array: &StringViewArray,
+ n_array: &Int64Array,
+) -> Result<ArrayRef> {
+ let len = n_array.len();
+
+ let views = string_view_array.views();
+ // Every string in StringViewArray has one corresponding view in `views`
+ debug_assert!(views.len() == string_view_array.len());
+
+ // Compose null buffer at once
+ let string_nulls = string_view_array.nulls();
+ let n_nulls = n_array.nulls();
+ let new_nulls = NullBuffer::union(string_nulls, n_nulls);
+
+ let new_views = (0..len)
+ .map(|idx| {
+ let view = views[idx];
+
+ let is_valid = match &new_nulls {
+ Some(nulls_buf) => nulls_buf.is_valid(idx),
+ None => true,
+ };
+
+ if is_valid {
+ let string: &str = string_view_array.value(idx);
+ let n = n_array.value(idx);
+
+ let new_offset = right_byte_length(string, n);
+ let result_bytes = &string.as_bytes()[new_offset..];
+
+ if result_bytes.len() > 12 {
+ let byte_view = ByteView::from(view);
+ // Reuse buffer, but adjust offset and length
+ make_view(
+ result_bytes,
+ byte_view.buffer_index,
+ byte_view.offset + new_offset as u32,
+ )
+ } else {
+ // inline value does not need block id or offset
+ make_view(result_bytes, 0, 0)
+ }
+ } else {
+ // For nulls, keep the original view
+ view
+ }
+ })
+ .collect::<Vec<u128>>();
+
+ // Buffers are unchanged
+ let result = StringViewArray::try_new(
+ ScalarBuffer::from(new_views),
+ Vec::from(string_view_array.data_buffers()),
+ new_nulls,
+ )?;
+ Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Calculate the byte length of the substring of last `n` chars from string `string`
+/// (or all but first `|n|` chars if n is negative)
+fn right_byte_length(string: &str, n: i64) -> usize {
Review Comment:
Unfortunately, it's quite different.
- `left_byte_length` and `right_byte_length` are almost symmetric with flipping the sign of the `n` argument, except for the `0` case.
- The side of lookup in the byte array - from the left or from the middle.
- The string view is built differently (length adjustment vs. offset).
So, having the generic implementation wouldn't be that helpful - plenty of ifs all around.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#discussion_r2751925845
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,133 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // Extract starting from `byte_length` bytes from a byte-indexed slice
+ Some(&string[byte_length..])
+ }
_ => None,
})
.collect::<GenericStringArray<T>>();
Ok(Arc::new(result) as ArrayRef)
}
+/// `right` implementation for StringViewArray
+fn right_impl_view(
+ string_view_array: &StringViewArray,
+ n_array: &Int64Array,
+) -> Result<ArrayRef> {
+ let len = n_array.len();
+
+ let views = string_view_array.views();
+ // Every string in StringViewArray has one corresponding view in `views`
+ debug_assert!(views.len() == string_view_array.len());
+
+ // Compose null buffer at once
+ let string_nulls = string_view_array.nulls();
+ let n_nulls = n_array.nulls();
+ let new_nulls = NullBuffer::union(string_nulls, n_nulls);
+
+ let new_views = (0..len)
+ .map(|idx| {
+ let view = views[idx];
+
+ let is_valid = match &new_nulls {
+ Some(nulls_buf) => nulls_buf.is_valid(idx),
+ None => true,
+ };
+
+ if is_valid {
+ let string: &str = string_view_array.value(idx);
+ let n = n_array.value(idx);
+
+ let new_offset = right_byte_length(string, n);
+ let result_bytes = &string.as_bytes()[new_offset..];
+
+ if result_bytes.len() > 12 {
+ let byte_view = ByteView::from(view);
+ // Reuse buffer, but adjust offset and length
+ make_view(
+ result_bytes,
+ byte_view.buffer_index,
+ byte_view.offset + new_offset as u32,
+ )
+ } else {
+ // inline value does not need block id or offset
+ make_view(result_bytes, 0, 0)
+ }
Review Comment:
Thank you for the review, @Jefffrey !
This call could be simplified, based on the implementation (the inline buffer is correctly padded with zeroes by `make_view`).
I created a ticket #20103 for follow-ups and created the unified implementation. I'll push it after the merge.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#discussion_r2750438870
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,140 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // println!(
+ // "Input string: {}, n: {} -> byte_length: {} -> {}",
+ // &string,
+ // n,
+ // byte_length,
+ // &string[byte_length..]
+ // );
+ // Extract starting from `byte_length` bytes from a byte-indexed slice
+ Some(&string[byte_length..])
+ }
_ => None,
})
.collect::<GenericStringArray<T>>();
Ok(Arc::new(result) as ArrayRef)
}
+/// `right` implementation for StringViewArray
+fn right_impl_view(
+ string_view_array: &StringViewArray,
+ n_array: &Int64Array,
+) -> Result<ArrayRef> {
+ let len = n_array.len();
+
+ let views = string_view_array.views();
+ // Every string in StringViewArray has one corresponding view in `views`
+ debug_assert!(views.len() == string_view_array.len());
+
+ // Compose null buffer at once
+ let string_nulls = string_view_array.nulls();
+ let n_nulls = n_array.nulls();
+ let new_nulls = NullBuffer::union(string_nulls, n_nulls);
+
+ let new_views = (0..len)
+ .map(|idx| {
+ let view = views[idx];
+
+ let is_valid = match &new_nulls {
+ Some(nulls_buf) => nulls_buf.is_valid(idx),
+ None => true,
+ };
+
+ if is_valid {
+ let string: &str = string_view_array.value(idx);
+ let n = n_array.value(idx);
+
+ let new_offset = right_byte_length(string, n);
+ let result_bytes = &string.as_bytes()[new_offset..];
+
+ if result_bytes.len() > 12 {
+ let byte_view = ByteView::from(view);
+ // Reuse buffer, but adjust offset and length
+ make_view(
+ result_bytes,
+ byte_view.buffer_index,
+ byte_view.offset + new_offset as u32,
+ )
+ } else {
+ // inline value does not need block id or offset
+ make_view(result_bytes, 0, 0)
+ }
+ } else {
+ // For nulls, keep the original view
+ view
+ }
+ })
+ .collect::<Vec<u128>>();
+
+ // Buffers are unchanged
+ let result = StringViewArray::try_new(
+ ScalarBuffer::from(new_views),
+ Vec::from(string_view_array.data_buffers()),
+ new_nulls,
+ )?;
+ Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Calculate the byte length of the substring of last `n` chars from string `string`
+/// (or all but first `|n|` chars if n is negative)
+fn right_byte_length(string: &str, n: i64) -> usize {
Review Comment:
I do think we could deduplicate somewhat, and perhaps having this function return a `Range` instead of just a `usize` might make it more feasible; but can explore this in a followup 👍
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,133 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // Extract starting from `byte_length` bytes from a byte-indexed slice
+ Some(&string[byte_length..])
+ }
_ => None,
})
.collect::<GenericStringArray<T>>();
Ok(Arc::new(result) as ArrayRef)
}
+/// `right` implementation for StringViewArray
+fn right_impl_view(
+ string_view_array: &StringViewArray,
+ n_array: &Int64Array,
+) -> Result<ArrayRef> {
+ let len = n_array.len();
+
+ let views = string_view_array.views();
+ // Every string in StringViewArray has one corresponding view in `views`
+ debug_assert!(views.len() == string_view_array.len());
+
+ // Compose null buffer at once
+ let string_nulls = string_view_array.nulls();
+ let n_nulls = n_array.nulls();
+ let new_nulls = NullBuffer::union(string_nulls, n_nulls);
+
+ let new_views = (0..len)
+ .map(|idx| {
+ let view = views[idx];
+
+ let is_valid = match &new_nulls {
+ Some(nulls_buf) => nulls_buf.is_valid(idx),
+ None => true,
+ };
+
+ if is_valid {
+ let string: &str = string_view_array.value(idx);
+ let n = n_array.value(idx);
+
+ let new_offset = right_byte_length(string, n);
+ let result_bytes = &string.as_bytes()[new_offset..];
+
+ if result_bytes.len() > 12 {
+ let byte_view = ByteView::from(view);
+ // Reuse buffer, but adjust offset and length
+ make_view(
+ result_bytes,
+ byte_view.buffer_index,
+ byte_view.offset + new_offset as u32,
+ )
+ } else {
+ // inline value does not need block id or offset
+ make_view(result_bytes, 0, 0)
+ }
Review Comment:
```suggestion
let byte_view = ByteView::from(view);
make_view(
result_bytes,
byte_view.buffer_index,
byte_view.offset + new_offset as u32,
)
```
We could probably avoid this outside if check since `make_view` already checks this for us
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #20069:
URL: https://github.com/apache/datafusion/pull/20069
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#discussion_r2749792956
##########
datafusion/functions/src/unicode/right.rs:
##########
@@ -119,58 +119,140 @@ impl ScalarUDFImpl for RightFunc {
}
}
-/// Returns last n characters in the string, or when n is negative, returns all but first |n| characters.
+/// Returns right n characters in the string, or when n is negative, returns all but first |n| characters.
/// right('abcde', 2) = 'de'
+/// right('abcde', -2) = 'cde'
/// The implementation uses UTF-8 code points as characters
-fn right<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn right(args: &[ArrayRef]) -> Result<ArrayRef> {
let n_array = as_int64_array(&args[1])?;
- if args[0].data_type() == &DataType::Utf8View {
- // string_view_right(args)
- let string_array = as_string_view_array(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
- } else {
- // string_right::<T>(args)
- let string_array = &as_generic_string_array::<T>(&args[0])?;
- right_impl::<T, _>(&mut string_array.iter(), n_array)
+
+ match args[0].data_type() {
+ DataType::Utf8 => {
+ let string_array = as_generic_string_array::<i32>(&args[0])?;
+ right_impl::<i32, _>(string_array, n_array)
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_generic_string_array::<i64>(&args[0])?;
+ right_impl::<i64, _>(string_array, n_array)
+ }
+ DataType::Utf8View => {
+ let string_view_array = as_string_view_array(&args[0])?;
+ right_impl_view(string_view_array, n_array)
+ }
+ _ => exec_err!("Not supported"),
}
}
-// Currently the return type can only be Utf8 or LargeUtf8, to reach fully support, we need
-// to edit the `get_optimal_return_type` in utils.rs to make the udfs be able to return Utf8View
-// See https://github.com/apache/datafusion/issues/11790#issuecomment-2283777166
+/// `right` implementation for strings
fn right_impl<'a, T: OffsetSizeTrait, V: ArrayAccessor<Item = &'a str>>(
- string_array_iter: &mut ArrayIter<V>,
+ string_array: V,
n_array: &Int64Array,
) -> Result<ArrayRef> {
- let result = string_array_iter
+ let iter = ArrayIter::new(string_array);
+ let result = iter
.zip(n_array.iter())
.map(|(string, n)| match (string, n) {
- (Some(string), Some(n)) => match n.cmp(&0) {
- Ordering::Less => Some(
- string
- .chars()
- .skip(n.unsigned_abs() as usize)
- .collect::<String>(),
- ),
- Ordering::Equal => Some("".to_string()),
- Ordering::Greater => Some(
- string
- .chars()
- .skip(max(string.chars().count() as i64 - n, 0) as usize)
- .collect::<String>(),
- ),
- },
+ (Some(string), Some(n)) => {
+ let byte_length = right_byte_length(string, n);
+ // println!(
+ // "Input string: {}, n: {} -> byte_length: {} -> {}",
+ // &string,
+ // n,
+ // byte_length,
+ // &string[byte_length..]
+ // );
+ // Extract starting from `byte_length` bytes from a byte-indexed slice
+ Some(&string[byte_length..])
+ }
_ => None,
})
.collect::<GenericStringArray<T>>();
Ok(Arc::new(result) as ArrayRef)
}
+/// `right` implementation for StringViewArray
+fn right_impl_view(
+ string_view_array: &StringViewArray,
+ n_array: &Int64Array,
+) -> Result<ArrayRef> {
+ let len = n_array.len();
+
+ let views = string_view_array.views();
+ // Every string in StringViewArray has one corresponding view in `views`
+ debug_assert!(views.len() == string_view_array.len());
+
+ // Compose null buffer at once
+ let string_nulls = string_view_array.nulls();
+ let n_nulls = n_array.nulls();
+ let new_nulls = NullBuffer::union(string_nulls, n_nulls);
+
+ let new_views = (0..len)
+ .map(|idx| {
+ let view = views[idx];
+
+ let is_valid = match &new_nulls {
+ Some(nulls_buf) => nulls_buf.is_valid(idx),
+ None => true,
+ };
+
+ if is_valid {
+ let string: &str = string_view_array.value(idx);
+ let n = n_array.value(idx);
+
+ let new_offset = right_byte_length(string, n);
+ let result_bytes = &string.as_bytes()[new_offset..];
+
+ if result_bytes.len() > 12 {
+ let byte_view = ByteView::from(view);
+ // Reuse buffer, but adjust offset and length
+ make_view(
+ result_bytes,
+ byte_view.buffer_index,
+ byte_view.offset + new_offset as u32,
+ )
+ } else {
+ // inline value does not need block id or offset
+ make_view(result_bytes, 0, 0)
+ }
+ } else {
+ // For nulls, keep the original view
+ view
+ }
+ })
+ .collect::<Vec<u128>>();
+
+ // Buffers are unchanged
+ let result = StringViewArray::try_new(
+ ScalarBuffer::from(new_views),
+ Vec::from(string_view_array.data_buffers()),
+ new_nulls,
+ )?;
+ Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Calculate the byte length of the substring of last `n` chars from string `string`
+/// (or all but first `|n|` chars if n is negative)
+fn right_byte_length(string: &str, n: i64) -> usize {
+ match n.cmp(&0) {
+ Ordering::Less => string
+ .char_indices()
+ .nth(n.unsigned_abs() as usize)
+ .map(|(index, _)| index)
+ .unwrap_or(string.len()),
+ Ordering::Equal => string.len(),
+ Ordering::Greater => string
+ .char_indices()
+ .nth_back(n.unsigned_abs() as usize - 1)
Review Comment:
For string arrays, we support 64-bit offsets on 32-bit platforms, but the string will be limited in 32-bit size. I added saturation, so `left` would return the whole string anyway.
For string views, Arrow provides 32-bit views only. Since we cannot construct a large string view as input, it won't be a problem.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#issuecomment-3820892396
cc @Jefffrey
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#issuecomment-3832852317
Thanks @theirix & @martin-g
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: optimise right for byte access and StringView [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on PR #20069:
URL: https://github.com/apache/datafusion/pull/20069#issuecomment-3828928479
What do you think of reworking both left and right benches to a single file like for [trim.rs](https://github.com/apache/datafusion/blob/main/datafusion/functions/benches/trim.rs)?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org