alamb commented on code in PR #27202:
URL: https://github.com/apache/datafusion/pull/27202#discussion_r4501379618
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -7,15 +7,15 @@
//! Arrow's comparison kernels work on the bit pattern of a float,
//! i.e. IEEE 754 totalOrder. DataFusion deviates from that in one
//! respect: `+0.0` and `-0.0` are equal, hash the same and fall into the same
-//! group. See [`normalize_float`], [`normalize_floats`] and
+//! group. See [`normalize_float`], [`normalize`] and
Review Comment:
normalize is now consistent with `ScalarValue::normalize`
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -149,20 +153,38 @@ pub fn normalize_floats(array: &ArrayRef) -> ArrayRef {
.expect("rewriting float leaves preserves the array
layout"),
)
}
- _ => Arc::clone(array),
+ _ => array,
}
}
-fn normalize_primitive<T>(array: &ArrayRef) -> ArrayRef
+/// Like [`normalize`], but borrows the array and so always copies the values
+/// buffer when there is something to normalize.
+#[deprecated(
+ since = "57.0.0",
+ note = "use `datafusion_common::float_semantics::normalize` instead"
+)]
+pub fn normalize_floats(array: &ArrayRef) -> ArrayRef {
+ normalize(Arc::clone(array))
+}
+
+fn normalize_primitive<T>(array: ArrayRef) -> ArrayRef
where
T: ArrowPrimitiveType,
T::Native: FloatSemantics,
{
let arr = array.as_primitive::<T>();
if !arr.values().iter().any(|v| v.needs_normalization()) {
- return Arc::clone(array);
+ return array;
}
- Arc::new(arr.unary::<_, T>(normalize_float))
+ // Drop the `ArrayRef` so the values buffer is uniquely owned (when nothing
+ // else refers to it) and can be rewritten in place.
+ let arr = arr.clone();
+ drop(array);
+ let normalized = match arr.unary_mut(normalize_float) {
Review Comment:
here is the attempt to reuse the allocation
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -1516,10 +1516,10 @@ fn fsl_values_row_number(list_size: i32, array_len:
usize) -> Result<Int32Array>
/// Replace `-0.0` with `+0.0` in any `Float16`, `Float32`, or `Float64` array.
#[deprecated(
since = "56.0.0",
- note = "use `datafusion_common::float_semantics::normalize_floats` instead"
+ note = "use `datafusion_common::float_semantics::normalize` instead"
)]
pub fn normalize_float_zero(array: &ArrayRef) -> ArrayRef {
- crate::float_semantics::normalize_floats(array)
+ crate::float_semantics::normalize(Arc::clone(array))
Review Comment:
The old signature forces a `clone` anyways
```rust
pub fn normalize_floats(array: &ArrayRef) -> ArrayRef {
```
This now just makes it explicit
##########
datafusion/expr-common/src/columnar_value.rs:
##########
@@ -113,6 +114,19 @@ impl ColumnarValue {
}
}
+ /// Applies DataFusion's [float equality semantics] to this value, so that
+ /// `-0.0` becomes `+0.0`. Values without floats are returned unchanged.
+ ///
+ /// [float equality semantics]: datafusion_common::float_semantics
+ pub fn normalize(self) -> Self {
Review Comment:
this pattern appeared twice in the code, so I have centralized it to a new
method
##########
datafusion/physical-expr-common/src/datum.rs:
##########
@@ -134,7 +127,7 @@ pub fn compare_with_eq(
fn normalize_nested_float_zero(array: &dyn Array) -> Option<ArrayRef> {
has_float_leaf(array.data_type())
- .then(|| normalize_floats(&make_array(array.to_data())))
+ .then(|| float_semantics::normalize(make_array(array.to_data())))
Review Comment:
remove a clone of Array ref 🤷 probably not a big deal
##########
datafusion/physical-expr-common/src/datum.rs:
##########
@@ -76,19 +76,12 @@ pub fn apply_cmp(
// floats, which treats `-0.0` and `+0.0` as distinct. Normalize float
// operands so SQL semantics (`+0.0 == -0.0`) hold. No-op for
// non-float types.
- let lhs = normalize_cmp_input(lhs);
- let rhs = normalize_cmp_input(rhs);
+ let lhs = lhs.clone().normalize();
+ let rhs = rhs.clone().normalize();
apply(&lhs, &rhs, |l, r| Ok(Arc::new(f(l, r)?)))
}
}
-fn normalize_cmp_input(cv: &ColumnarValue) -> ColumnarValue {
Review Comment:
Moved to `ColumnarValue::normalize`
##########
datafusion/physical-expr-common/src/datum.rs:
##########
@@ -76,19 +76,12 @@ pub fn apply_cmp(
// floats, which treats `-0.0` and `+0.0` as distinct. Normalize float
// operands so SQL semantics (`+0.0 == -0.0`) hold. No-op for
// non-float types.
- let lhs = normalize_cmp_input(lhs);
- let rhs = normalize_cmp_input(rhs);
+ let lhs = lhs.clone().normalize();
Review Comment:
the old code clones too
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -72,13 +71,6 @@ fn supports_arrow_eq(dt: &DataType) -> bool {
}
}
-fn normalize_in_list_value(value: ColumnarValue) -> ColumnarValue {
Review Comment:
second copy of `ColumnarValue::normalize`
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1057,7 +1057,12 @@ impl<const STREAMING: bool> GroupValues for
GroupValuesColumn<STREAMING> {
let normalized: Option<Vec<ArrayRef>> = cols
.iter()
.any(|col| col.data_type().is_nested() &&
has_float_leaf(col.data_type()))
- .then(|| cols.iter().map(normalize_floats).collect());
+ .then(|| {
+ cols.iter()
+ .cloned()
Review Comment:
Note these are clones of `ArrayRef`
##########
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;
Review Comment:
this was unecessarily verbose -- the point is the rationable for normalizing
join keys, we don't need a re-explanation for how normalize_floats works
--
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]
