You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Kev1n8 (via GitHub)" <gi...@apache.org> on 2024/08/17 16:37:58 UTC
[PR] Improve StringView support for SUBSTR [datafusion]
Kev1n8 opened a new pull request, #12044:
URL: https://github.com/apache/datafusion/pull/12044
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #12031
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
1. When it comes to `StringVIew` in `SUBSTR`, operate directly on the views instead of generating a new `String`.
2. I took the liberty of making the sqllogictest treat `Utf8View` column as `Text`.
3. Added a bench file to record differences in the following conditions:
both the input and output of `SUBSTR` is larger or smaller than 12B and the input is larger while the output is smaller than 12B. Also, compare `Utf8View`, `Utf8`, and `LargeUtf8` with each other.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
5. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
no
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1727098779
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
+ let str = string_view_array.value_unchecked(idx);
+ let (start, end) =
+ get_true_start_count(str, start as usize, -1);
+ builder.append_view_unchecked(
+ view.buffer_index,
+ view.offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ // Safety:
+ // (1) original bytes are valid utf-8,
+ // (2) we do not slice on utf-8 codepoint
+ unsafe {
+ let bytes =
+ StringViewArray::inline_value(raw, length as usize);
Review Comment:
Maybe we could add a function like `append_view_u128_unchecked(view: u128)` in arrow/src/builder/generic_bytes_view_builder.rs to simply add a view with a given `u128`. Then the whole process would be:
1. Get the str of the view by `value_unchecked`, then get the [start, end)
2. sub_view = ~~if end-start>12 substr_large_view() else substr_small_view()~~ make_view()
3. call `appned_view_u128_unchecked(sub_view)` on the builder
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2295040890
I think the PR is looking good, left some comments
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2294908528
The following is my latest bench report of `SUBSTR`:
```
SHORTER THAN 12/substr_string_view [size=1024, strlen=12]
time: [29.675 µs 29.964 µs 30.402 µs]
change: [-0.1382% +0.9754% +2.5969%] (p = 0.20 > 0.05)
No change in performance detected.
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high severe
SHORTER THAN 12/substr_string [size=1024, strlen=12]
time: [59.403 µs 59.823 µs 60.292 µs]
change: [+0.2182% +0.9242% +1.7185%] (p = 0.04 < 0.05)
Change within noise threshold.
SHORTER THAN 12/substr_large_string [size=1024, strlen=12]
time: [41.462 µs 41.775 µs 42.064 µs]
change: [-0.7169% +0.3030% +1.2513%] (p = 0.57 > 0.05)
No change in performance detected.
LONGER THAN 12/substr_string_view [size=1024, count=64, strlen=128]
time: [68.409 µs 68.896 µs 69.437 µs]
change: [-22.017% -10.028% +1.0113%] (p = 0.23 > 0.05)
No change in performance detected.
LONGER THAN 12/substr_string [size=1024, count=64, strlen=128]
time: [101.98 µs 102.20 µs 102.55 µs]
change: [-0.1908% +0.0183% +0.3687%] (p = 0.92 > 0.05)
No change in performance detected.
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high severe
LONGER THAN 12/substr_large_string [size=1024, count=64, strlen=128]
time: [101.90 µs 102.32 µs 102.80 µs]
change: [+0.1753% +0.5640% +1.0536%] (p = 0.01 < 0.05)
Change within noise threshold.
SRC_LEN > 12, SUB_LEN < 12/substr_string_view [size=1024, count=6, strlen=128]
time: [15.031 µs 15.180 µs 15.331 µs]
SRC_LEN > 12, SUB_LEN < 12/substr_string [size=1024, count=6, strlen=128]
time: [33.738 µs 34.057 µs 34.370 µs]
SRC_LEN > 12, SUB_LEN < 12/substr_large_string [size=1024, count=6, strlen=128]
time: [33.509 µs 34.122 µs 35.075 µs]
Found 2 outliers among 10 measurements (20.00%)
1 (10.00%) high mild
1 (10.00%) high severe
SHORTER THAN 12/substr_string_view [size=4096, strlen=12]
time: [120.22 µs 121.43 µs 122.70 µs]
change: [+1.3982% +2.3499% +3.4501%] (p = 0.00 < 0.05)
Performance has regressed.
SHORTER THAN 12/substr_string [size=4096, strlen=12]
time: [237.06 µs 251.39 µs 268.81 µs]
change: [+1.6838% +7.2415% +14.130%] (p = 0.04 < 0.05)
Performance has regressed.
SHORTER THAN 12/substr_large_string [size=4096, strlen=12]
time: [163.27 µs 164.24 µs 165.40 µs]
change: [-0.2464% +0.3706% +1.0851%] (p = 0.34 > 0.05)
No change in performance detected.
Found 2 outliers among 10 measurements (20.00%)
2 (20.00%) high mild
LONGER THAN 12/substr_string_view [size=4096, count=64, strlen=128]
time: [275.69 µs 277.81 µs 280.16 µs]
change: [+0.9658% +1.8029% +2.6242%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high mild
LONGER THAN 12/substr_string [size=4096, count=64, strlen=128]
time: [416.84 µs 419.95 µs 423.71 µs]
change: [-1.6174% -0.0917% +1.2731%] (p = 0.91 > 0.05)
No change in performance detected.
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high mild
LONGER THAN 12/substr_large_string [size=4096, count=64, strlen=128]
time: [419.60 µs 421.94 µs 424.48 µs]
change: [+1.5505% +2.0977% +2.7392%] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high mild
SRC_LEN > 12, SUB_LEN < 12/substr_string_view [size=4096, count=6, strlen=128]
time: [59.933 µs 60.122 µs 60.355 µs]
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high mild
SRC_LEN > 12, SUB_LEN < 12/substr_string [size=4096, count=6, strlen=128]
time: [133.30 µs 134.66 µs 135.97 µs]
SRC_LEN > 12, SUB_LEN < 12/substr_large_string [size=4096, count=6, strlen=128]
time: [131.37 µs 141.42 µs 158.42 µs]
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high severe
substr_index_array_array_1000
time: [64.130 µs 68.632 µs 73.376 µs]
change: [-16.971% -10.021% -2.4160%] (p = 0.01 < 0.05)
Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
4 (4.00%) high mild
10 (10.00%) high severe
```
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1721027725
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -77,7 +78,11 @@ impl ScalarUDFImpl for SubstrFunc {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- utf8_to_str_type(&arg_types[0], "substr")
+ if arg_types[0] == DataType::Utf8View {
+ Ok(DataType::Utf8View)
+ } else {
+ utf8_to_str_type(&arg_types[0], "substr")
+ }
Review Comment:
I added `optmized_utf8_to_str_type` in utils.rs, wonder if it's proper.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1721021641
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -168,10 +330,34 @@ where
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
+fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
Review Comment:
Thanks for pointing this out. I didn't realize it earlier.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720954368
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -168,10 +330,34 @@ where
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
+fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
Review Comment:
I think this function adds an unecessary level of indirection
Rather than having`substr` dispatch to `calculate_substr` which then dispatches to `calculate_string` or `calculate_string_view` I suspect the code would be easier to follow
##########
datafusion/sqllogictest/test_files/arrow_typeof.slt:
##########
@@ -424,7 +424,7 @@ select arrow_cast([1, 2, 3], 'FixedSizeList(3, Int64)');
[1, 2, 3]
# Tests for Utf8View
-query ?T
+query TT
Review Comment:
I believe it is correct -- I actually have a PR to do the same change in https://github.com/apache/datafusion/pull/12033 but it hasn't merged yet. It is good to do it in this PR instead. Thank you
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -77,7 +78,11 @@ impl ScalarUDFImpl for SubstrFunc {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- utf8_to_str_type(&arg_types[0], "substr")
+ if arg_types[0] == DataType::Utf8View {
+ Ok(DataType::Utf8View)
+ } else {
+ utf8_to_str_type(&arg_types[0], "substr")
+ }
Review Comment:
Maybe we can make a second function like `optmized_utf8_to_str_type` -- with comments that make it clear that the difference between that and `utf8_to_str_type` if if the string function has a specialized implementation for string view.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720849600
##########
datafusion/functions/benches/substr.rs:
##########
@@ -0,0 +1,206 @@
+// 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 arrow::array::{ArrayRef, Int64Array, OffsetSizeTrait};
+use arrow::util::bench_util::{
+ create_string_array_with_len, create_string_view_array_with_len,
+};
+use criterion::{black_box, criterion_group, criterion_main, Criterion, SamplingMode};
+use datafusion_expr::ColumnarValue;
+use datafusion_functions::unicode;
+use std::sync::Arc;
+use std::time::Duration;
+
+fn create_args_without_count<O: OffsetSizeTrait>(
+ size: usize,
+ str_len: usize,
+ start_half_way: bool,
+ use_string_view: bool,
+) -> Vec<ColumnarValue> {
+ let start_array = Arc::new(Int64Array::from(
+ (0..size)
+ .map(|_| {
+ if start_half_way {
+ (str_len / 2) as i64
+ } else {
+ 1i64
+ }
+ })
+ .collect::<Vec<_>>(),
+ ));
+
+ if use_string_view {
+ let string_array =
+ Arc::new(create_string_view_array_with_len(size, 0.1, str_len, false));
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(start_array),
+ ]
+ } else {
+ let string_array =
+ Arc::new(create_string_array_with_len::<O>(size, 0.1, str_len));
+
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(Arc::clone(&start_array) as ArrayRef),
+ ]
+ }
+}
+
+fn create_args_with_count<O: OffsetSizeTrait>(
+ size: usize,
+ str_len: usize,
+ count_max: usize,
+ use_string_view: bool,
+) -> Vec<ColumnarValue> {
+ let start_array =
+ Arc::new(Int64Array::from((0..size).map(|_| 1).collect::<Vec<_>>()));
+ let count = count_max.min(str_len) as i64;
+ let count_array = Arc::new(Int64Array::from(
+ (0..size).map(|_| count).collect::<Vec<_>>(),
+ ));
+
+ if use_string_view {
+ let string_array =
+ Arc::new(create_string_view_array_with_len(size, 0.1, str_len, false));
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(start_array),
+ ColumnarValue::Array(count_array),
+ ]
+ } else {
+ let string_array =
+ Arc::new(create_string_array_with_len::<O>(size, 0.1, str_len));
+
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(Arc::clone(&start_array) as ArrayRef),
+ ColumnarValue::Array(Arc::clone(&count_array) as ArrayRef),
+ ]
+ }
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+ let substr = unicode::substr();
+ for size in [1024, 4096] {
+ // string_len = 12, substring_len=6 (see `create_args_without_count`)
+ let len = 12;
+ let mut group = c.benchmark_group("SHORTER THAN 12");
+ group.sampling_mode(SamplingMode::Flat);
+ group.sample_size(10);
+ group.measurement_time(Duration::from_secs(10));
Review Comment:
Is there a reason we tune the sampling parameter?
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2308886041
Thanks @Kev1n8 ad @XiangpengHao -- I am running some benchmarks on this now.
BTW I was thinking that once we have completed https://github.com/apache/datafusion/issues/12119 we could potentially make `substr` *always* return `Utf8View` -- as it should always be better to use the views rather than Utf8Array
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1728473902
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
+ let str = string_view_array.value_unchecked(idx);
+ let (start, end) =
+ get_true_start_count(str, start as usize, -1);
+ builder.append_view_unchecked(
+ view.buffer_index,
+ view.offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ // Safety:
+ // (1) original bytes are valid utf-8,
+ // (2) we do not slice on utf-8 codepoint
+ unsafe {
+ let bytes =
+ StringViewArray::inline_value(raw, length as usize);
Review Comment:
> Reading this code carefully I do think is the case but it is quite implicit (it knows that calling append_value on a string that is less than 12 bytes)
Yes
> Maybe we could add a function like append_view_u128_unchecked(view: u128)
I like this direction.
The current `append_view_unchecked` is basically only for long strings (as it asks for a block id), and we don't have a similar method for short strings, what's why we call `append_value` as a workaround.
We can probably have a function called `append_inlined_view_unchecked(len: usize, prefix: &[u8])` to make it the intention clearer
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1722375767
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -107,11 +112,170 @@ pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn calculate_string_view(
+ string_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let buffer_index = (*raw >> 64) as u32;
Review Comment:
I made https://github.com/apache/arrow-rs/pull/6275# to export this structure publically
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1727067357
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
Review Comment:
Maybe we could use `value_unchecked` to retrieve `str` in both cases.
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
+ let str = string_view_array.value_unchecked(idx);
+ let (start, end) =
+ get_true_start_count(str, start as usize, -1);
+ builder.append_view_unchecked(
+ view.buffer_index,
+ view.offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ // Safety:
+ // (1) original bytes are valid utf-8,
+ // (2) we do not slice on utf-8 codepoint
+ unsafe {
+ let bytes =
+ StringViewArray::inline_value(raw, length as usize);
Review Comment:
Maybe we could add a function like `append_view_u128_unchecked(view: u128)` in arrow/src/builder/generic_bytes_view_builder.rs to simply add a view with a given `u128`. Then the whole process would be:
1. Get the str of the view by `value_unchecked`, then get the [start, end)
2. `sub_view = if end-start>12 substr_large_view() else substr_small_view()`
3. call `appned_view_u128_unchecked(sub_view)` on the builder
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1722379426
##########
datafusion/functions/src/utils.rs:
##########
@@ -71,6 +71,20 @@ macro_rules! get_optimal_return_type {
// `utf8_to_str_type`: returns either a Utf8 or LargeUtf8 based on the input type size.
get_optimal_return_type!(utf8_to_str_type, DataType::LargeUtf8, DataType::Utf8);
+// `optimized_utf8_to_str_type`: returns Utf8View when the string function has a specialised
+// implementation for StringView which returns Utf8View.
+pub(crate) fn optimized_utf8_to_str_type(
+ arg_type: &DataType,
+ name: &str,
+) -> Result<DataType> {
+ let support_list = ["substr"];
+ if support_list.contains(&name) {
+ Ok(DataType::Utf8View)
+ } else {
+ utf8_to_str_type(arg_type, name)
+ }
+}
Review Comment:
🤔 since this function simply returns `Utf8View` regardless of the input types, maybe we should just change `substr()` to directly return `Ok(DataType::Utf8View)` -- that would probably be the simplest
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720949851
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -107,11 +112,170 @@ pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn calculate_string_view(
+ string_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let buffer_index = (*raw >> 64) as u32;
Review Comment:
I think we can add an explclit dependence on `arrow-data` similar to https://github.com/apache/datafusion/blob/950dc73c7f763ae8dc56c0a99de864dff444f22b/Cargo.toml#L86
In general I think it would be better if the `arrow` crate re-exported these various structures (so DataFusion could depend only on `arrow` rather than all the sub crates)
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2334778040
🚀
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1725668538
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
+ let str = string_view_array.value_unchecked(idx);
+ let (start, end) =
+ get_true_start_count(str, start as usize, -1);
+ builder.append_view_unchecked(
+ view.buffer_index,
+ view.offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ // Safety:
+ // (1) original bytes are valid utf-8,
+ // (2) we do not slice on utf-8 codepoint
+ unsafe {
+ let bytes =
+ StringViewArray::inline_value(raw, length as usize);
Review Comment:
I would expect that we never had to call `append_value` -- as I think the `substr` calculation can be done entirely in terms of the views (and never modify the values in the buffer). Reading this code carefully I do think is the case but it is quite implicit (it knows that calling append_value on a string that is less than 12 bytes) will not push values values into the buffer
I wonder if we could refactor some of this code into functions with names
Perhaps something like
```rust
/// Modify a `view` with length <= 12 bytes so it reflects the substring [start..end]
fn substr_small_view(len: usize, view: u128, start: usize, end: size) -> u128 { ... }
/// Modify a `view` with length > 12 bytes so it reflects the substring [start..end]
fn substr_large_view(len: usize, view: u128, start: usize, end: size) -> u128 { ... }
```
Then we could simply updated the views directly
However, the `StringViewBuilder` doesn't seem to allow for that at the moment....
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
Review Comment:
There seems like there is some non trivial duplication here and the clauses below
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1722708925
##########
datafusion/functions/src/utils.rs:
##########
@@ -71,6 +71,20 @@ macro_rules! get_optimal_return_type {
// `utf8_to_str_type`: returns either a Utf8 or LargeUtf8 based on the input type size.
get_optimal_return_type!(utf8_to_str_type, DataType::LargeUtf8, DataType::Utf8);
+// `optimized_utf8_to_str_type`: returns Utf8View when the string function has a specialised
+// implementation for StringView which returns Utf8View.
+pub(crate) fn optimized_utf8_to_str_type(
+ arg_type: &DataType,
+ name: &str,
+) -> Result<DataType> {
+ let support_list = ["substr"];
+ if support_list.contains(&name) {
+ Ok(DataType::Utf8View)
+ } else {
+ utf8_to_str_type(arg_type, name)
+ }
+}
Review Comment:
yeah, I think so too.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720863104
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -107,11 +112,170 @@ pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn calculate_string_view(
+ string_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let buffer_index = (*raw >> 64) as u32;
+ let offset = (*raw >> 96) as u32;
+ let str = string_array.value(idx);
+ let (start, end) = get_true_start_count(str, start as usize, -1);
+ // Safety: builder is guaranteed to have corresponding blocks
+ unsafe {
+ builder.append_view_unchecked(
+ buffer_index,
+ offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ let bytes = ((*raw >> 32) & u128::MAX).to_le_bytes();
+ let str = match std::str::from_utf8(&bytes[..length as usize]) {
+ Ok(str) => {
+ // Extract str[start, end) by char
+ let (start, end) = get_true_start_count(
+ str,
+ start as usize,
+ length as i64,
+ );
+ &str[start..end]
+ }
+ _ => {
+ return exec_err!(
+ "failed to convert inline bytes to &str."
+ )
+ }
+ };
+ builder.append_value(str);
+ }
+ } else {
+ builder.append_null();
+ }
+ }
+ }
+ 2 => {
+ let count_array = as_int64_array(&args[1])?;
+ for (idx, ((raw, start), count)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .zip(count_array.iter())
+ .enumerate()
+ {
+ if let (Some(start), Some(count)) = (start, count) {
+ let length = *raw as u32;
+ let start = (start - 1).max(0) as usize;
Review Comment:
```suggestion
let start = (start as usize).saturating_sub(1);
```
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #12044:
URL: https://github.com/apache/datafusion/pull/12044
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720862601
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -107,11 +112,170 @@ pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn calculate_string_view(
+ string_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let buffer_index = (*raw >> 64) as u32;
+ let offset = (*raw >> 96) as u32;
+ let str = string_array.value(idx);
+ let (start, end) = get_true_start_count(str, start as usize, -1);
+ // Safety: builder is guaranteed to have corresponding blocks
+ unsafe {
+ builder.append_view_unchecked(
+ buffer_index,
+ offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ let bytes = ((*raw >> 32) & u128::MAX).to_le_bytes();
Review Comment:
```suggestion
let bytes = unsafe { StringViewArray::inline_value(raw, length as usize) };
```
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2331481739
Sorry -- here are the results of my benchmark run -- TLDR is this looks great to me
```
++ critcmp main stringview-output-for-substr
group main stringview-output-for-substr
----- ---- ----------------------------
LONGER THAN 12/substr_large_string [size=1024, count=64, strlen=128] 1.01 147.7±0.16µs ? ?/sec 1.00 146.4±0.10µs ? ?/sec
LONGER THAN 12/substr_large_string [size=4096, count=64, strlen=128] 1.00 590.8±0.14µs ? ?/sec 1.00 589.1±1.54µs ? ?/sec
LONGER THAN 12/substr_string [size=1024, count=64, strlen=128] 1.05 154.9±0.25µs ? ?/sec 1.00 148.0±0.11µs ? ?/sec
LONGER THAN 12/substr_string [size=4096, count=64, strlen=128] 1.08 636.7±0.81µs ? ?/sec 1.00 591.5±0.27µs ? ?/sec
LONGER THAN 12/substr_string_view [size=1024, count=64, strlen=128] 1.50 158.2±0.20µs ? ?/sec 1.00 105.4±0.04µs ? ?/sec
LONGER THAN 12/substr_string_view [size=4096, count=64, strlen=128] 1.50 629.0±0.18µs ? ?/sec 1.00 420.6±0.12µs ? ?/sec
SHORTER THAN 12/substr_large_string [size=1024, strlen=12] 1.00 56.8±0.02µs ? ?/sec 1.03 58.3±0.93µs ? ?/sec
SHORTER THAN 12/substr_large_string [size=4096, strlen=12] 1.00 223.8±0.30µs ? ?/sec 1.02 227.3±0.05µs ? ?/sec
SHORTER THAN 12/substr_string [size=1024, strlen=12] 1.05 89.3±0.05µs ? ?/sec 1.00 84.9±0.04µs ? ?/sec
SHORTER THAN 12/substr_string [size=4096, strlen=12] 1.05 351.7±0.32µs ? ?/sec 1.00 335.2±0.10µs ? ?/sec
SHORTER THAN 12/substr_string_view [size=1024, strlen=12] 2.82 58.4±0.04µs ? ?/sec 1.00 20.7±0.01µs ? ?/sec
SHORTER THAN 12/substr_string_view [size=4096, strlen=12] 2.79 227.8±0.05µs ? ?/sec 1.00 81.6±0.04µs ? ?/sec
SRC_LEN > 12, SUB_LEN < 12/substr_large_string [size=1024, count=6, strlen=128] 1.00 40.9±0.01µs ? ?/sec 1.00 41.1±0.03µs ? ?/sec
SRC_LEN > 12, SUB_LEN < 12/substr_large_string [size=4096, count=6, strlen=128] 1.01 157.6±0.13µs ? ?/sec 1.00 156.3±0.13µs ? ?/sec
SRC_LEN > 12, SUB_LEN < 12/substr_string [size=1024, count=6, strlen=128] 1.06 43.9±0.11µs ? ?/sec 1.00 41.5±0.08µs ? ?/sec
SRC_LEN > 12, SUB_LEN < 12/substr_string [size=4096, count=6, strlen=128] 1.07 169.7±0.03µs ? ?/sec 1.00 158.4±0.09µs ? ?/sec
SRC_LEN > 12, SUB_LEN < 12/substr_string_view [size=1024, count=6, strlen=128] 1.74 43.7±0.01µs ? ?/sec 1.00 25.1±0.04µs ? ?/sec
SRC_LEN > 12, SUB_LEN < 12/substr_string_view [size=4096, count=6, strlen=128] 1.69 166.4±0.07µs ? ?/sec 1.00 98.6±0.06µs ? ?/sec
```
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2299678521
BTW I hope to help this PR along tomorrow
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1728509523
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
+ let str = string_view_array.value_unchecked(idx);
+ let (start, end) =
+ get_true_start_count(str, start as usize, -1);
+ builder.append_view_unchecked(
+ view.buffer_index,
+ view.offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ // Safety:
+ // (1) original bytes are valid utf-8,
+ // (2) we do not slice on utf-8 codepoint
+ unsafe {
+ let bytes =
+ StringViewArray::inline_value(raw, length as usize);
Review Comment:
The thing is that I think retrieving the `&str` by `value_unchecked()` is a must-do to calculate the `[start, end)`, then the `sub_view: u128` could be created based on the original view using `make_view`. I'll make some commits.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2331589621
Filed https://github.com/apache/datafusion/issues/12338 to track the idea of using StringViewArray always
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2316111793
> BTW I was thinking that once we have completed https://github.com/apache/datafusion/issues/12119 we could potentially make substr always return Utf8View -- as it should always be better to use the views rather than Utf8Array
Sounds great. And I think I can also improve the `TRIM` function in this way too, it's quite similar to `SUBSTR`.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720808406
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -77,7 +78,11 @@ impl ScalarUDFImpl for SubstrFunc {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- utf8_to_str_type(&arg_types[0], "substr")
+ if arg_types[0] == DataType::Utf8View {
+ Ok(DataType::Utf8View)
+ } else {
+ utf8_to_str_type(&arg_types[0], "substr")
+ }
Review Comment:
This might be a temporary implementation in my opinion. We might need to modify `utf8_to_str_type` eventually.
##########
datafusion/sqllogictest/test_files/arrow_typeof.slt:
##########
@@ -424,7 +424,7 @@ select arrow_cast([1, 2, 3], 'FixedSizeList(3, Int64)');
[1, 2, 3]
# Tests for Utf8View
-query ?T
+query TT
Review Comment:
It was after I made sqllogictest treat utf8view as Text that I found this way, by using a `?`, to represent unknown column. I wonder if my change in normalize.rs is correct.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720861699
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -107,11 +112,170 @@ pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn calculate_string_view(
+ string_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let buffer_index = (*raw >> 64) as u32;
Review Comment:
We should use `ByteView` from arrow-rs:
https://github.com/apache/arrow-rs/blob/27789d7c9abb50796a4042e7e193703efe3c95b3/arrow-data/src/byte_view.rs#L44-L54
But ByteView is behind `arrow-data`, which is not explicitly depended by DataFusion, what's your opinion? @alamb
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720849326
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -77,7 +78,11 @@ impl ScalarUDFImpl for SubstrFunc {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- utf8_to_str_type(&arg_types[0], "substr")
+ if arg_types[0] == DataType::Utf8View {
+ Ok(DataType::Utf8View)
+ } else {
+ utf8_to_str_type(&arg_types[0], "substr")
+ }
Review Comment:
I agree
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1727098779
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -89,29 +94,193 @@ impl ScalarUDFImpl for SubstrFunc {
}
}
+/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
+/// substr('alphabet', 3) = 'phabet'
+/// substr('alphabet', 3, 2) = 'ph'
+/// The implementation uses UTF-8 code points as characters
pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
match args[0].data_type() {
DataType::Utf8 => {
let string_array = args[0].as_string::<i32>();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_substr::<_, i32>(string_array, &args[1..])
}
DataType::LargeUtf8 => {
let string_array = args[0].as_string::<i64>();
- calculate_substr::<_, i64>(string_array, &args[1..])
+ string_substr::<_, i64>(string_array, &args[1..])
}
DataType::Utf8View => {
let string_array = args[0].as_string_view();
- calculate_substr::<_, i32>(string_array, &args[1..])
+ string_view_substr(string_array, &args[1..])
}
- other => exec_err!("Unsupported data type {other:?} for function substr"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function substr,\
+ expected Utf8View, Utf8 or LargeUtf8."
+ ),
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn string_view_substr(
+ string_view_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_view_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_view_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let view = ByteView::from(*raw);
+
+ // Safety:
+ // 1. idx < string_array.views.size()
+ // 2. builder is guaranteed to have corresponding blocks
+ unsafe {
+ let str = string_view_array.value_unchecked(idx);
+ let (start, end) =
+ get_true_start_count(str, start as usize, -1);
+ builder.append_view_unchecked(
+ view.buffer_index,
+ view.offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ // Safety:
+ // (1) original bytes are valid utf-8,
+ // (2) we do not slice on utf-8 codepoint
+ unsafe {
+ let bytes =
+ StringViewArray::inline_value(raw, length as usize);
Review Comment:
Maybe we could add a function like `append_view_u128_unchecked(view: u128)` in arrow/src/builder/generic_bytes_view_builder.rs to simply add a view with a given `u128`. Then the whole process would be:
1. Get the str of the view by `value_unchecked`, then get the [start, end)
2. `sub_view = ~~if end-start>12 substr_large_view() else substr_small_view()`~~ make_view()
3. call `appned_view_u128_unchecked(sub_view)` on the builder
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2331524216
I also merged up from main for this PR to get the arrow 53 release (and thus the changes to add arrow-data as a dependency were removed)
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#issuecomment-2305212409
> * Get the str of the view by `value_unchecked`, then get the [start, end)
> * `sub_view = if end-start>12 substr_large_view() else substr_small_view()`
> * call `appned_view_u128_unchecked(sub_view)` on th
I can open a PR for that `append_u128` function in arrow if it's considered a good idea, and then adjust this PR.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1722391921
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -107,11 +112,170 @@ pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn calculate_string_view(
+ string_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let buffer_index = (*raw >> 64) as u32;
Review Comment:
It is also kind of annoying that the use of `inline_value` always comes with an immediate call to `from_utf8_unchecked`
```rust
let bytes =
StringViewArray::inline_value(raw, length as usize);
let str = std::str::from_utf8_unchecked(
&bytes[..length as usize],
);
```
I'll try and make a PR in arrow-rs to do that too
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720862894
##########
datafusion/functions/src/unicode/substr.rs:
##########
@@ -107,11 +112,170 @@ pub fn substr(args: &[ArrayRef]) -> Result<ArrayRef> {
}
}
-/// Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).)
-/// substr('alphabet', 3) = 'phabet'
-/// substr('alphabet', 3, 2) = 'ph'
-/// The implementation uses UTF-8 code points as characters
-fn calculate_substr<'a, V, T>(string_array: V, args: &[ArrayRef]) -> Result<ArrayRef>
+// Return the exact byte index for [start, end), set count to -1 to ignore count
+fn get_true_start_count(input: &str, start: usize, count: i64) -> (usize, usize) {
+ let (mut st, mut ed) = (input.len(), input.len());
+ let mut start_counting = false;
+ let mut cnt = 0;
+ for (char_cnt, (byte_cnt, _)) in input.char_indices().enumerate() {
+ if char_cnt == start {
+ st = byte_cnt;
+ if count != -1 {
+ start_counting = true;
+ } else {
+ break;
+ }
+ }
+ if start_counting {
+ if cnt == count {
+ ed = byte_cnt;
+ break;
+ }
+ cnt += 1;
+ }
+ }
+ (st, ed)
+}
+
+// The decoding process refs the trait at: arrow/arrow-data/src/byte_view.rs:44
+// From<u128> for ByteView
+fn calculate_string_view(
+ string_array: &StringViewArray,
+ args: &[ArrayRef],
+) -> Result<ArrayRef> {
+ let mut builder = StringViewBuilder::new();
+ // Copy all blocks from input
+ for block in string_array.data_buffers() {
+ builder.append_block(block.clone());
+ }
+
+ let start_array = as_int64_array(&args[0])?;
+
+ match args.len() {
+ 1 => {
+ for (idx, (raw, start)) in string_array
+ .views()
+ .iter()
+ .zip(start_array.iter())
+ .enumerate()
+ {
+ if let Some(start) = start {
+ let length = *raw as u32;
+ let start = (start - 1).max(0);
+
+ // Operate according to the length of bytes
+ if length == 0 {
+ builder.append_null();
+ } else if length > 12 {
+ let buffer_index = (*raw >> 64) as u32;
+ let offset = (*raw >> 96) as u32;
+ let str = string_array.value(idx);
+ let (start, end) = get_true_start_count(str, start as usize, -1);
+ // Safety: builder is guaranteed to have corresponding blocks
+ unsafe {
+ builder.append_view_unchecked(
+ buffer_index,
+ offset + start as u32,
+ // guarantee that end-offset >= 0 for end <= str.len()
+ (end - start) as u32,
+ );
+ }
+ } else {
+ let bytes = ((*raw >> 32) & u128::MAX).to_le_bytes();
+ let str = match std::str::from_utf8(&bytes[..length as usize]) {
Review Comment:
Here we should use `from_utf8_unchecked` to avoid expensive utf8-validation.
Safety: (1) original bytes are valid utf-8, (2) we do not slice on utf-8 codepoint, i.e., won't break a multi-byte utf-8 code. The substr is also utf-8.
--
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] Improve StringView support for SUBSTR [datafusion]
Posted by "Kev1n8 (via GitHub)" <gi...@apache.org>.
Kev1n8 commented on code in PR #12044:
URL: https://github.com/apache/datafusion/pull/12044#discussion_r1720887680
##########
datafusion/functions/benches/substr.rs:
##########
@@ -0,0 +1,206 @@
+// 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 arrow::array::{ArrayRef, Int64Array, OffsetSizeTrait};
+use arrow::util::bench_util::{
+ create_string_array_with_len, create_string_view_array_with_len,
+};
+use criterion::{black_box, criterion_group, criterion_main, Criterion, SamplingMode};
+use datafusion_expr::ColumnarValue;
+use datafusion_functions::unicode;
+use std::sync::Arc;
+use std::time::Duration;
+
+fn create_args_without_count<O: OffsetSizeTrait>(
+ size: usize,
+ str_len: usize,
+ start_half_way: bool,
+ use_string_view: bool,
+) -> Vec<ColumnarValue> {
+ let start_array = Arc::new(Int64Array::from(
+ (0..size)
+ .map(|_| {
+ if start_half_way {
+ (str_len / 2) as i64
+ } else {
+ 1i64
+ }
+ })
+ .collect::<Vec<_>>(),
+ ));
+
+ if use_string_view {
+ let string_array =
+ Arc::new(create_string_view_array_with_len(size, 0.1, str_len, false));
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(start_array),
+ ]
+ } else {
+ let string_array =
+ Arc::new(create_string_array_with_len::<O>(size, 0.1, str_len));
+
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(Arc::clone(&start_array) as ArrayRef),
+ ]
+ }
+}
+
+fn create_args_with_count<O: OffsetSizeTrait>(
+ size: usize,
+ str_len: usize,
+ count_max: usize,
+ use_string_view: bool,
+) -> Vec<ColumnarValue> {
+ let start_array =
+ Arc::new(Int64Array::from((0..size).map(|_| 1).collect::<Vec<_>>()));
+ let count = count_max.min(str_len) as i64;
+ let count_array = Arc::new(Int64Array::from(
+ (0..size).map(|_| count).collect::<Vec<_>>(),
+ ));
+
+ if use_string_view {
+ let string_array =
+ Arc::new(create_string_view_array_with_len(size, 0.1, str_len, false));
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(start_array),
+ ColumnarValue::Array(count_array),
+ ]
+ } else {
+ let string_array =
+ Arc::new(create_string_array_with_len::<O>(size, 0.1, str_len));
+
+ vec![
+ ColumnarValue::Array(string_array),
+ ColumnarValue::Array(Arc::clone(&start_array) as ArrayRef),
+ ColumnarValue::Array(Arc::clone(&count_array) as ArrayRef),
+ ]
+ }
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+ let substr = unicode::substr();
+ for size in [1024, 4096] {
+ // string_len = 12, substring_len=6 (see `create_args_without_count`)
+ let len = 12;
+ let mut group = c.benchmark_group("SHORTER THAN 12");
+ group.sampling_mode(SamplingMode::Flat);
+ group.sample_size(10);
+ group.measurement_time(Duration::from_secs(10));
Review Comment:
I actually simply copied these settings from benches/repeat. I'm not very sure about this.
--
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