You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kevinmingtarja (via GitHub)" <gi...@apache.org> on 2024/04/06 08:01:40 UTC
[PR] Optimize performance of substr_index and add tests [arrow-datafusion]
kevinmingtarja opened a new pull request, #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973
## Which issue does this PR close?
Closes #9890
## Rationale for this change
We were using dynamic dispatch when deciding whether to do `string.split()` or `string.rsplit()` (depending on `count`) because the exact type of iterator is not known at compile time, plus it allocates memory on the heap for the boxed iterator, which introduces considerable overhead. Instead, we can replace this with an if else statement, which removes that overhead.
### Benchmarks
```bash
substr_index_array_array_1000
time: [87.589 µs 88.761 µs 90.840 µs]
change: [-29.073% -27.415% -25.725%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe
```
## What changes are included in this PR?
## Are these changes tested?
Yes. I added new unit tests as well for `substr_index` since there were none before.
## Are there any user-facing changes?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554656036
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,151 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
- (Some(string), Some(delimiter), Some(n)) => {
+ .for_each(|((string, delimiter), n)| {
+ if let (Some(string), Some(delimiter), Some(n)) = (string, delimiter, n) {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
- Some(string[..length].to_owned())
+ if let Some(substring) = string.get(..length) {
+ builder.append_value(substring);
+ }
} else {
- Some(string[string.len() - length..].to_owned())
+ #[allow(clippy::collapsible_else_if)]
Review Comment:
I felt like collapsing this to `else if let Some(...) = ...` makes it less readable, which is why I override it.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554977408
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,154 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
- (Some(string), Some(delimiter), Some(n)) => {
+ .for_each(|((string, delimiter), n)| {
+ if let (Some(string), Some(delimiter), Some(n)) = (string, delimiter, n) {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
- Some(string[..length].to_owned())
+ if let Some(substring) = string.get(..length) {
+ builder.append_value(substring);
+ }
} else {
- Some(string[string.len() - length..].to_owned())
+ #[allow(clippy::collapsible_else_if)]
+ if let Some(substring) =
+ string.get(string.len().saturating_sub(length)..)
+ {
+ builder.append_value(substring);
+ }
}
}
- _ => None,
- })
- .collect::<GenericStringArray<T>>();
+ });
+
+ if builder.is_empty() {
+ builder.append_null();
Review Comment:
Got it, I've added a new test in that file.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554649963
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -112,18 +112,22 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
return Some(String::new());
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
Some(string[..length].to_owned())
Review Comment:
Actually, since we haven't merged this, let me do this real quick!
And thanks for pointing me to this, I wasn't aware of Arrow's StringBuilder API before.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#issuecomment-2041586755
Nice work! If there is another function that you think could use a look at for performance I'll be happy to write the benchmark for testing.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#issuecomment-2041036627
CI [failure](https://github.com/apache/arrow-datafusion/actions/runs/8579761480/job/23515164131?pr=9973) seems unrelated:
```
error: failed to run `rustc` to learn about target-specific information
Caused by:
process didn't exit successfully: `sccache /usr/local/rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/rustc - --crate-name ___ --print=file-names -C debuginfo=line-tables-only -C incremental=false --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit status: 2)
--- stderr
sccache: error: Timed out waiting for server startup. Maybe the remote service is unreachable?
Run with SCCACHE_LOG=debug SCCACHE_NO_DAEMON=1 to get more information
Error: Process completed with exit code 101.
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#issuecomment-2041521770
FYI @Omega359
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554655376
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -112,18 +112,22 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
return Some(String::new());
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
Some(string[..length].to_owned())
Review Comment:
Awesome, managed to achieve 50% speedup now in total!
```bash
substr_index_array_array_1000
time: [57.082 µs 57.407 µs 57.770 µs]
change: [-53.118% -52.034% -51.077%] (p = 0.00 < 0.05)
Performance has improved.
```
I've just pushed the new commit.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554977306
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,154 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
- (Some(string), Some(delimiter), Some(n)) => {
+ .for_each(|((string, delimiter), n)| {
+ if let (Some(string), Some(delimiter), Some(n)) = (string, delimiter, n) {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
- Some(string[..length].to_owned())
+ if let Some(substring) = string.get(..length) {
Review Comment:
Ah I see, got it. I've made a fix a for 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554566944
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -112,18 +112,22 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
return Some(String::new());
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
Some(string[..length].to_owned())
Review Comment:
This code looks much nicer -- thank you @kevinmingtarja
Something else you could do is avoid the `String` creation here entirely -- creating `String`s for each row is non ideal for two reasons:
1. It does an allocation
2. It needs to be copied *again* to the final output
Specifically, using https://docs.rs/arrow/latest/arrow/array/type.StringBuilder.html you could build up the output directly
So something like
```rust
let mut builder = StringBuilder::new();
...
if n > 0 {
builder.append_value(string[..length]));
} else {
builder.append_value(string[string.len() - length..])
}
...
```
The key is to avoid the `to_owned()` call
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#issuecomment-2043448603
🚀
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554977408
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,154 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
- (Some(string), Some(delimiter), Some(n)) => {
+ .for_each(|((string, delimiter), n)| {
+ if let (Some(string), Some(delimiter), Some(n)) = (string, delimiter, n) {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
- Some(string[..length].to_owned())
+ if let Some(substring) = string.get(..length) {
+ builder.append_value(substring);
+ }
} else {
- Some(string[string.len() - length..].to_owned())
+ #[allow(clippy::collapsible_else_if)]
+ if let Some(substring) =
+ string.get(string.len().saturating_sub(length)..)
+ {
+ builder.append_value(substring);
+ }
}
}
- _ => None,
- })
- .collect::<GenericStringArray<T>>();
+ });
+
+ if builder.is_empty() {
+ builder.append_null();
Review Comment:
Got it, I've added a new NULL row in that test.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1555017594
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,151 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
+ .for_each(|((string, delimiter), n)| match (string, delimiter, n) {
(Some(string), Some(delimiter), Some(n)) => {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
Review Comment:
Nice, I would factor our this code into internal function
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1555145475
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,151 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
+ .for_each(|((string, delimiter), n)| match (string, delimiter, n) {
(Some(string), Some(delimiter), Some(n)) => {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
Review Comment:
got it
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#issuecomment-2041047117
> CI [failure](https://github.com/apache/arrow-datafusion/actions/runs/8579761480/job/23515164131?pr=9973) seems unrelated:
Agreed -- I restarted the job
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1554927300
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,154 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
- (Some(string), Some(delimiter), Some(n)) => {
+ .for_each(|((string, delimiter), n)| {
+ if let (Some(string), Some(delimiter), Some(n)) = (string, delimiter, n) {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
- Some(string[..length].to_owned())
+ if let Some(substring) = string.get(..length) {
+ builder.append_value(substring);
+ }
} else {
- Some(string[string.len() - length..].to_owned())
+ #[allow(clippy::collapsible_else_if)]
+ if let Some(substring) =
+ string.get(string.len().saturating_sub(length)..)
+ {
+ builder.append_value(substring);
+ }
}
}
- _ => None,
- })
- .collect::<GenericStringArray<T>>();
+ });
+
+ if builder.is_empty() {
+ builder.append_null();
Review Comment:
I think append null needs to be called on each row (not just once at the end) -- this will work fine for tests that only use a single row array (like these unit tests) but likely won't work for arrays with multiple rows
It might be useful to add some tests here for NULL inputs :
https://github.com/apache/arrow-datafusion/blob/1a002bccd420ff91ec149ee1ba9c42061510f906/datafusion/sqllogictest/test_files/functions.slt#L939-L954
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,154 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
- (Some(string), Some(delimiter), Some(n)) => {
+ .for_each(|((string, delimiter), n)| {
+ if let (Some(string), Some(delimiter), Some(n)) = (string, delimiter, n) {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
- Some(string[..length].to_owned())
+ if let Some(substring) = string.get(..length) {
Review Comment:
For each row here, I think you either need to call `append_value` or `append_null` -- the code as I read it will not call `append_null` for non matching rows. I'll suggest some more tests 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1555005836
##########
datafusion/sqllogictest/test_files/functions.slt:
##########
@@ -954,6 +955,14 @@ SELECT str, n, substring_index(str, '.', n) AS c FROM
) AS occurrences(n)
ORDER BY str DESC, n;
----
+NULL -100 NULL
Review Comment:
💯 thank you
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,151 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
+ .for_each(|((string, delimiter), n)| match (string, delimiter, n) {
(Some(string), Some(delimiter), Some(n)) => {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
} else {
- Box::new(string.rsplit(delimiter))
+ let splitted = string.rsplit(delimiter);
+ splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len()
};
- let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
if n > 0 {
- Some(string[..length].to_owned())
+ match string.get(..length) {
+ Some(substring) => builder.append_value(substring),
+ None => builder.append_null(),
+ }
} else {
- Some(string[string.len() - length..].to_owned())
+ match string.get(string.len().saturating_sub(length)..) {
+ Some(substring) => builder.append_value(substring),
+ None => builder.append_null(),
+ }
}
}
- _ => None,
- })
- .collect::<GenericStringArray<T>>();
+ _ => builder.append_null(),
+ });
+
+ Ok(Arc::new(builder.finish()) as ArrayRef)
Review Comment:
👨🍳 👌 -- very nice
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#discussion_r1555017714
##########
datafusion/functions/src/unicode/substrindex.rs:
##########
@@ -101,38 +101,151 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let delimiter_array = as_generic_string_array::<T>(&args[1])?;
let count_array = as_int64_array(&args[2])?;
- let result = string_array
+ let mut builder = StringBuilder::new();
+ string_array
.iter()
.zip(delimiter_array.iter())
.zip(count_array.iter())
- .map(|((string, delimiter), n)| match (string, delimiter, n) {
+ .for_each(|((string, delimiter), n)| match (string, delimiter, n) {
(Some(string), Some(delimiter), Some(n)) => {
// In MySQL, these cases will return an empty string.
if n == 0 || string.is_empty() || delimiter.is_empty() {
- return Some(String::new());
+ builder.append_value("");
+ return;
}
- let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
+ let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
+ let length = if n > 0 {
+ let splitted = string.split(delimiter);
+ splitted
Review Comment:
can be done in follow up PR @kevinmingtarja
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize performance of substr_index and add tests [arrow-datafusion]
Posted by "kevinmingtarja (via GitHub)" <gi...@apache.org>.
kevinmingtarja commented on PR #9973:
URL: https://github.com/apache/arrow-datafusion/pull/9973#issuecomment-2041726352
Thanks for the help and review @alamb, @comphead, and thanks @Omega359 for writing the benchmark!
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org