Skip to content

Commit 7bfe690

Browse files
authored
Correctly handle Nan/Inf comparison in ALP between reduce (#8126)
Found in #8105. ALP comparisons to NaN/Inf can be trivially folded since those values must exist in patches --------- Signed-off-by: Robert Kruszewski <github@robertk.io>
1 parent 39fd9e6 commit 7bfe690

1 file changed

Lines changed: 77 additions & 8 deletions

File tree

encodings/alp/src/alp/compute/between.rs

Lines changed: 77 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
use std::fmt::Debug;
55

6+
use num_traits::Bounded;
67
use vortex_array::ArrayRef;
78
use vortex_array::ArrayView;
89
use vortex_array::IntoArray;
@@ -19,6 +20,7 @@ use vortex_error::VortexResult;
1920

2021
use crate::ALP;
2122
use crate::ALPFloat;
23+
use crate::Exponents;
2224
use crate::alp::array::ALPArrayExt;
2325
use crate::alp::array::ALPArraySlotsExt;
2426
use crate::match_each_alp_float_ptype;
@@ -72,14 +74,18 @@ where
7274
// and we encode into value below enc_below(value) < value < x, in which case the comparison
7375
// becomes enc(value) < x. See `alp_scalar_compare` for more details.
7476
// note that if the value doesn't encode than value != x, so must use strict comparison.
75-
let (lower_enc, lower_strict) = T::encode_single(lower, exponents)
76-
.map(|x| (x, options.lower_strict))
77-
.unwrap_or_else(|| (T::encode_below(lower, exponents), StrictComparison::Strict));
77+
let Some((lower_enc, lower_strict)) =
78+
encode_lower_bound::<T>(lower, exponents, options.lower_strict)
79+
else {
80+
return Ok(ConstantArray::new(Scalar::bool(false, nullability), array.len()).into_array());
81+
};
7882

7983
// the upper value `x { < | <= } value` similarly encodes or `x < value < enc_above(value())`
80-
let (upper_enc, upper_strict) = T::encode_single(upper, exponents)
81-
.map(|x| (x, options.upper_strict))
82-
.unwrap_or_else(|| (T::encode_above(upper, exponents), StrictComparison::Strict));
84+
let Some((upper_enc, upper_strict)) =
85+
encode_upper_bound::<T>(upper, exponents, options.upper_strict)
86+
else {
87+
return Ok(ConstantArray::new(Scalar::bool(false, nullability), array.len()).into_array());
88+
};
8389

8490
let options = BetweenOptions {
8591
lower_strict,
@@ -93,14 +99,50 @@ where
9399
)
94100
}
95101

102+
fn encode_lower_bound<T: ALPFloat>(
103+
lower: T,
104+
exponents: Exponents,
105+
strict: StrictComparison,
106+
) -> Option<(T::ALPInt, StrictComparison)> {
107+
if NativePType::is_nan(lower) || NativePType::is_infinite(lower) {
108+
return NativePType::is_lt(lower, T::zero())
109+
.then_some((T::ALPInt::min_value(), StrictComparison::NonStrict));
110+
}
111+
112+
Some(
113+
T::encode_single(lower, exponents)
114+
.map(|x| (x, strict))
115+
.unwrap_or_else(|| (T::encode_below(lower, exponents), StrictComparison::Strict)),
116+
)
117+
}
118+
119+
fn encode_upper_bound<T: ALPFloat>(
120+
upper: T,
121+
exponents: Exponents,
122+
strict: StrictComparison,
123+
) -> Option<(T::ALPInt, StrictComparison)> {
124+
if NativePType::is_nan(upper) || NativePType::is_infinite(upper) {
125+
return NativePType::is_gt(upper, T::zero())
126+
.then_some((T::ALPInt::max_value(), StrictComparison::NonStrict));
127+
}
128+
129+
Some(
130+
T::encode_single(upper, exponents)
131+
.map(|x| (x, strict))
132+
.unwrap_or_else(|| (T::encode_above(upper, exponents), StrictComparison::Strict)),
133+
)
134+
}
135+
96136
#[cfg(test)]
97137
mod tests {
138+
use vortex_array::IntoArray;
98139
use vortex_array::LEGACY_SESSION;
99140
use vortex_array::VortexSessionExecute;
100-
use vortex_array::arrays::BoolArray;
141+
use vortex_array::arrays::ConstantArray;
101142
use vortex_array::arrays::PrimitiveArray;
102143
use vortex_array::assert_arrays_eq;
103144
use vortex_array::dtype::Nullability;
145+
use vortex_array::scalar::Scalar;
104146
use vortex_array::scalar_fn::fns::between::BetweenOptions;
105147
use vortex_array::scalar_fn::fns::between::StrictComparison;
106148

@@ -118,7 +160,11 @@ mod tests {
118160
) {
119161
let res =
120162
between_impl(arr.as_view(), lower, upper, Nullability::Nullable, options).unwrap();
121-
assert_arrays_eq!(res, BoolArray::from_iter([Some(expected)]));
163+
assert_arrays_eq!(
164+
res,
165+
ConstantArray::new(Scalar::bool(expected, res.dtype().nullability()), arr.len())
166+
.into_array()
167+
);
122168
}
123169

124170
#[test]
@@ -188,4 +234,27 @@ mod tests {
188234
true,
189235
);
190236
}
237+
238+
#[test]
239+
fn non_finite_bounds_use_total_order() {
240+
let mut ctx = LEGACY_SESSION.create_execution_ctx();
241+
let array = PrimitiveArray::from_iter([1.234f32; 10]);
242+
let encoded = alp_encode(array.as_view(), None, &mut ctx).unwrap();
243+
assert!(encoded.patches().is_none());
244+
245+
let options = BetweenOptions {
246+
lower_strict: StrictComparison::NonStrict,
247+
upper_strict: StrictComparison::Strict,
248+
};
249+
250+
assert_between(&encoded, f32::from_bits(0xffffff5e), 2.0, &options, true);
251+
assert_between(&encoded, f32::NAN, 2.0, &options, false);
252+
assert_between(&encoded, f32::NEG_INFINITY, 2.0, &options, true);
253+
assert_between(&encoded, f32::INFINITY, 2.0, &options, false);
254+
255+
assert_between(&encoded, 0.0, f32::NAN, &options, true);
256+
assert_between(&encoded, 0.0, f32::from_bits(0xffffff5e), &options, false);
257+
assert_between(&encoded, 0.0, f32::INFINITY, &options, true);
258+
assert_between(&encoded, 0.0, f32::NEG_INFINITY, &options, false);
259+
}
191260
}

0 commit comments

Comments
 (0)