alamb commented on code in PR #10054:
URL:
https://github.com/apache/arrow-datafusion/pull/10054#discussion_r1564780071
##########
datafusion/functions/src/string/overlay.rs:
##########
@@ -88,10 +105,13 @@ pub fn overlay<T: OffsetSizeTrait>(args: &[ArrayRef]) ->
Result<ArrayRef> {
let characters_array = as_generic_string_array::<T>(&args[1])?;
let pos_num = as_int64_array(&args[2])?;
+ let characters_array_iter =
adaptive_array_iter(characters_array.iter());
Review Comment:
> According to the benchmark, there can be about a 12% performance
improvement 🚀 when there are 3 scalar args; however, when all args are arrays,
there is a 2% ~ 3% performance loss 😔. Does anyone have any ideas on how to
avoid this loss?
One reason it may be getting slower is that there is overhead now during
iteration (where it has to switch on `Left/Right` in each element
One thing to try would be to avoid the `Left/Right` iterator by
instantiating different loops depending on the input types
So something like
```rust
fn<CI, PI> overlay3(string_array: &StringArray, characters_array_iter: CI,
pos_num_iter: PI) -> Result<ArrayRef>
where
CI: Iterator<Item = char>,
PI: Iterator<Item =usize>
{
let result = string_array
.iter()
.zip(characters_array_iter)
.zip(pos_num_iter)
.map(|((string, characters), start_pos)| {
...
}
```
And then you need to instantiate versions based on lengths:
```rust
match (character_array.len(), pos_num.len()) {
(1, 1) => {
let c_value = character_array.next().expect("Contains a value");
let p_value = pos_num.next().expect("Contains a value");
overlay3(character_array, std::iter::repeat(c_value, len),
std;:iter::repeat(p_value, len))
(1, _) => {
let c_value = character_array.next().expect("Contains a value");
overlay3(character_array, std::iter::repeat(c_value, len),
pos_num.iter())
...
}
```
##########
datafusion/functions/src/string/overlay.rs:
##########
@@ -88,10 +105,13 @@ pub fn overlay<T: OffsetSizeTrait>(args: &[ArrayRef]) ->
Result<ArrayRef> {
let characters_array = as_generic_string_array::<T>(&args[1])?;
let pos_num = as_int64_array(&args[2])?;
+ let characters_array_iter =
adaptive_array_iter(characters_array.iter());
+ let pos_num_iter = adaptive_array_iter(pos_num.iter());
+
let result = string_array
.iter()
- .zip(characters_array.iter())
- .zip(pos_num.iter())
+ .zip(characters_array_iter)
+ .zip(pos_num_iter)
Review Comment:
While looking at the innter loop of this function, you can probably get a
major boost by avoiding `String::with_capacoty` and instead reuse the same
`String` on each element. It might also be possible to skip the copy by using
StringBuilder directly rather than copying to a temp string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
