Dandandan commented on a change in pull request #1374:
URL: https://github.com/apache/arrow-rs/pull/1374#discussion_r817936203
##########
File path: arrow/src/compute/kernels/aggregate.rs
##########
@@ -32,39 +32,26 @@ fn is_nan<T: ArrowNativeType + PartialOrd + Copy>(a: T) ->
bool {
!(a == a)
}
-/// Helper macro to perform min/max of strings
-fn min_max_string<T: StringOffsetSizeTrait, F: Fn(&str, &str) -> bool>(
- array: &GenericStringArray<T>,
- cmp: F,
-) -> Option<&str> {
+/// Helper function to perform min/max of strings
+fn min_max_string<T, F>(array: &GenericStringArray<T>, cmp: F) -> Option<&str>
+where
+ T: StringOffsetSizeTrait,
+ F: Fn(&str, &str) -> bool,
+{
let null_count = array.null_count();
if null_count == array.len() {
- return None;
- }
- let data = array.data();
- let mut n;
- if null_count == 0 {
- n = array.value(0);
- for i in 1..data.len() {
- let item = array.value(i);
- if cmp(n, item) {
- n = item;
- }
- }
+ None
+ } else if null_count == 0 {
+ (0..array.len())
+ .map(|i| array.value(i))
+ .reduce(|acc, item| if cmp(acc, item) { item } else { acc })
} else {
- n = "";
- let mut has_value = false;
-
- for i in 0..data.len() {
- let item = array.value(i);
- if data.is_valid(i) && (!has_value || cmp(n, item)) {
- has_value = true;
- n = item;
- }
- }
+ array
Review comment:
I think we could speed it up a bit more if we could iterate fast on the
non-null values.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
