Dandandan commented on a change in pull request #1374:
URL: https://github.com/apache/arrow-rs/pull/1374#discussion_r816528363
##########
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:
According to your benchmark result, this new way a bit slower than
before for nulls?
--
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
