Skip to content

Commit

Permalink
fix(rust, python): explicit nan comparison in min/max agg (#5403)
Browse files Browse the repository at this point in the history
  • Loading branch information
ritchie46 committed Nov 2, 2022
1 parent 982c10e commit 4bbff18
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
2 changes: 2 additions & 0 deletions polars/polars-arrow/src/kernels/rolling/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type WindowSize = usize;
type Len = usize;

#[inline]
/// NaN will be smaller than every valid value
pub fn compare_fn_nan_min<T>(a: &T, b: &T) -> Ordering
where
T: PartialOrd + IsFloat,
Expand All @@ -42,6 +43,7 @@ where
}

#[inline]
/// NaN will be larger than every valid value
pub fn compare_fn_nan_max<T>(a: &T, b: &T) -> Ordering
where
T: PartialOrd + IsFloat,
Expand Down
18 changes: 16 additions & 2 deletions polars/polars-core/src/chunked_array/ops/aggregate.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
//! Implementations of the ChunkAgg trait.
use std::cmp::Ordering;
use std::ops::Add;

use arrow::compute;
use arrow::types::simd::Simd;
use num::{Float, ToPrimitive};
use polars_arrow::kernels::rolling::{compare_fn_nan_max, compare_fn_nan_min};
use polars_arrow::prelude::QuantileInterpolOptions;

use crate::chunked_array::ChunkedArray;
Expand Down Expand Up @@ -88,7 +90,13 @@ where
IsSorted::Not => self
.downcast_iter()
.filter_map(compute::aggregate::min_primitive)
.fold_first_(|acc, v| if acc < v { acc } else { v }),
.fold_first_(|acc, v| {
if matches!(compare_fn_nan_max(&acc, &v), Ordering::Less) {
acc
} else {
v
}
}),
}
}

Expand All @@ -111,7 +119,13 @@ where
IsSorted::Not => self
.downcast_iter()
.filter_map(compute::aggregate::max_primitive)
.fold_first_(|acc, v| if acc > v { acc } else { v }),
.fold_first_(|acc, v| {
if matches!(compare_fn_nan_min(&acc, &v), Ordering::Greater) {
acc
} else {
v
}
}),
}
}

Expand Down

0 comments on commit 4bbff18

Please sign in to comment.