alamb commented on code in PR #27202:
URL: https://github.com/apache/datafusion/pull/27202#discussion_r4515488815
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2762,43 +2774,90 @@ impl ScalarValue {
)
}
- /// Applies DataFusion's floating-point equality semantics to this value,
- /// including floats inside nested and encoded values. Negative zero
becomes
- /// positive zero; nulls, NaN payloads and other values are preserved.
+ /// Applies DataFusion's [floating-point equality semantics] to this value
///
- /// See [`crate::float_semantics`] for the shared normalization rules.
- pub fn normalize(mut self) -> Self {
- use crate::float_semantics::{normalize_float, normalize_floats};
-
- fn normalize_array<A: Array + From<ArrayData> + 'static>(array: &mut
Arc<A>) {
- let original: ArrayRef = Arc::clone(array) as ArrayRef;
- let normalized = normalize_floats(&original);
- if !Arc::ptr_eq(&original, &normalized) {
- *array = Arc::new(downcast_array(normalized.as_ref()));
+ /// [floating-point equality semantics]: crate::float_semantics
+ pub fn normalize(self) -> Self {
+ match self {
+ ScalarValue::Float16(Some(v)) => {
+ ScalarValue::Float16(Some(normalize_float(v)))
}
- }
-
- fn normalize(scalar: &mut ScalarValue) {
- match scalar {
- ScalarValue::Float16(Some(value)) => *value =
normalize_float(*value),
- ScalarValue::Float32(Some(value)) => *value =
normalize_float(*value),
- ScalarValue::Float64(Some(value)) => *value =
normalize_float(*value),
- ScalarValue::FixedSizeList(array) => normalize_array(array),
- ScalarValue::List(array) => normalize_array(array),
- ScalarValue::LargeList(array) => normalize_array(array),
- ScalarValue::ListView(array) => normalize_array(array),
- ScalarValue::LargeListView(array) => normalize_array(array),
- ScalarValue::Struct(array) => normalize_array(array),
- ScalarValue::Map(array) => normalize_array(array),
- ScalarValue::Union(Some((_, value)), _, _)
- | ScalarValue::Dictionary(_, value)
- | ScalarValue::RunEndEncoded(_, _, value) => normalize(value),
- _ => {}
+ ScalarValue::Float32(Some(v)) => {
+ ScalarValue::Float32(Some(normalize_float(v)))
+ }
+ ScalarValue::Float64(Some(v)) => {
+ ScalarValue::Float64(Some(normalize_float(v)))
+ }
+ ScalarValue::FixedSizeList(array) => {
+ ScalarValue::FixedSizeList(normalize_array(array))
+ }
+ ScalarValue::List(array) =>
ScalarValue::List(normalize_array(array)),
+ ScalarValue::LargeList(array) => {
+ ScalarValue::LargeList(normalize_array(array))
+ }
+ ScalarValue::ListView(array) =>
ScalarValue::ListView(normalize_array(array)),
+ ScalarValue::LargeListView(array) => {
+ ScalarValue::LargeListView(normalize_array(array))
}
+ ScalarValue::Struct(array) =>
ScalarValue::Struct(normalize_array(array)),
+ ScalarValue::Map(array) =>
ScalarValue::Map(normalize_array(array)),
+ ScalarValue::Union(Some((type_id, value)), fields, mode) => {
+ ScalarValue::Union(
+ Some((type_id, Box::new(value.normalize()))),
+ fields,
+ mode,
+ )
+ }
+ ScalarValue::Dictionary(key_type, value) => {
+ ScalarValue::Dictionary(key_type, Box::new(value.normalize()))
+ }
+ ScalarValue::RunEndEncoded(run_ends, values, value) => {
+ ScalarValue::RunEndEncoded(run_ends, values,
Box::new(value.normalize()))
+ }
+ // Everything else holds no float, or is a null float
Review Comment:
list all other types explicitly so it is easy to verify what is not covered
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2723,17 +2723,13 @@ impl JoinKeyComparator {
.map(|((l, r), opts)| {
// `make_comparator` uses IEEE 754 totalOrder for floats and
// treats `-0.0` / `+0.0` as distinct. Normalize float arrays
- // so SMJ / piecewise-merge equi-keys honor SQL equality;
- // no-op (Arc::clone) for non-floats and for float arrays
- // that contain no `-0.0`. `normalize_floats` preserves
- // null positions, so the original null masks below remain
- // valid. A left side that is already normalized is used as is.
+ // to honor SQL equality;
Review Comment:
Added -- I think that is borderline obvious but not a bad thing to be
explicit about
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -272,10 +292,34 @@ mod tests {
#[test]
fn normalize_floats_reuses_normalized_input() {
let array: ArrayRef = Arc::new(Float64Array::from(vec![0.0, 1.0,
f64::NAN]));
- assert!(Arc::ptr_eq(&normalize_floats(&array), &array));
+ assert!(Arc::ptr_eq(&normalize(Arc::clone(&array)), &array));
let array: ArrayRef = Arc::new(Int32Array::from(vec![1, 2]));
- assert!(Arc::ptr_eq(&normalize_floats(&array), &array));
+ assert!(Arc::ptr_eq(&normalize(Arc::clone(&array)), &array));
+ }
+
+ #[test]
+ fn normalize_floats_rewrites_in_place_when_unique() {
Review Comment:
Thank you -- added in d0e18d7042 (though refactored to avoid as much
duplication)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
