Dandandan commented on code in PR #2183:
URL: https://github.com/apache/arrow-datafusion/pull/2183#discussion_r846838079
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -430,17 +431,17 @@ fn string_concat(left: ArrayRef, right: ArrayRef) ->
Result<ArrayRef> {
scalar_value => scalar_value.into_array(left.clone().len()),
};
let ignore_null_array =
ignore_null.as_any().downcast_ref::<StringArray>().unwrap();
- let result = (0..ignore_null_array.len())
+ let index_array = (0..ignore_null_array.len())
.into_iter()
.map(|index| {
if left.is_null(index) || right.is_null(index) {
None
} else {
- Some(ignore_null_array.value(index))
+ Some(index as u32)
}
})
- .collect::<StringArray>();
-
+ .collect::<UInt32Array>();
+ let result = take(ignore_null_array, &index_array, None)?;
Review Comment:
AIUC, the suggestion of @alamb might be roughly:
* Create the validity array based on `left_bitmap && right_bitmap` (this is
a fast operation, as it can be performed on 64 bits at once). A fast path could
be implemented for the null count being equal to 0 or being equal to length of
the array.
* Compute the concatenation based on left + right side (without needing
taking into account null values).
We should add some benches to see the improvements this brings.
In the arrow-rs kernels you could find some examples of this strategy
implemented, but usually this brings 2-10x improvements, depending on data type.
In arrow2 this strategy is applied quite a bit more consistently, see for
example the `substring` kernel:
https://github.com/jorgecarleitao/arrow2/blob/main/src/compute/substring.rs#L66
--
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]
