jayzhan211 commented on code in PR #27202:
URL: https://github.com/apache/datafusion/pull/27202#discussion_r4504343034
##########
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:
Optional cleanup: the shortened comment drops why the `l` / `r` null masks
used below are still valid, and it ends in `;`.
```suggestion
// `make_comparator` uses IEEE 754 totalOrder for floats and
// treats `-0.0` / `+0.0` as distinct. Normalize float arrays
// to honor SQL equality. Normalizing keeps null positions,
so
// the null masks of `l` and `r` below stay valid.
```
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -119,11 +119,12 @@ pub fn hash_input<T: FloatSemantics>(v: T) -> T {
/// Callers use this before handing float data to Arrow kernels that compare
/// bit patterns, so that `+0.0 = -0.0` holds.
///
-/// The common case - nothing to normalize - is allocation-free: a single
-/// read-only scan of the values buffer (auto-vectorizable to an OR-reduction)
-/// decides whether to fall through to the rewriting path. Only arrays that
-/// contain values to normalize pay for a new buffer.
-pub fn normalize_floats(array: &ArrayRef) -> ArrayRef {
+/// Optimized for the common case of nothing to normalize, by searching for
such
+/// values in a vectorized way.
+///
+/// If the values do require normalization, attempts to rewrite the values in
+/// place, if possible.
Review Comment:
Optional: could the doc say when the rewrite happens in place? Callers that
pass `Arc::clone(&array)` (most of them today) always get a copy.
```suggestion
/// If the values do require normalization, they are rewritten in place when
/// `array` holds the only reference to its values and validity buffers and
/// does not start at an offset into them; otherwise they are copied. Passing
/// `Arc::clone(&array)` therefore always copies.
```
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -1519,7 +1519,7 @@ fn fsl_values_row_number(list_size: i32, array_len:
usize) -> Result<Int32Array>
note = "use `datafusion_common::float_semantics::normalize_floats` instead"
Review Comment:
Optional cleanup: this note now sends users to `normalize_floats`, which
this PR deprecates too.
```suggestion
note = "use `datafusion_common::float_semantics::normalize` instead"
```
##########
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:
Optional: the in-place test has no case with nulls and no sliced case. I ran
both locally and they pass: a unique array with a null is rewritten in place,
and a unique slice at an offset falls back to a copy. Adding them would keep a
future change to `into_builder` from breaking this silently.
<details>
<summary>Test I ran on this branch (passes)</summary>
```rs
#[test]
fn normalize_in_place_with_nulls_and_slices() {
// Unique array with nulls: rewritten in place, null kept
let array: ArrayRef =
Arc::new(Float64Array::from(vec![Some(-0.0), None, Some(1.0)]));
let values_ptr = array.as_primitive::<Float64Type>().values().as_ptr();
let result = normalize(array);
let result = result.as_primitive::<Float64Type>();
assert_eq!(result.values().as_ptr(), values_ptr);
assert_eq!(result.value(0).to_bits(), 0.0_f64.to_bits());
assert!(result.is_null(1));
assert_eq!(result.null_count(), 1);
// Unique slice at an offset: copied, parent left as it was
let parent = Float64Array::from(vec![Some(5.0), Some(-0.0), None,
Some(2.0)]);
let result = normalize(Arc::new(parent.slice(1, 3)));
let result = result.as_primitive::<Float64Type>();
assert_eq!(result.len(), 3);
assert_eq!(result.value(0).to_bits(), 0.0_f64.to_bits());
assert!(result.is_null(1));
assert_eq!(result.value(2), 2.0);
assert_eq!(parent.value(1).to_bits(), (-0.0_f64).to_bits());
}
```
</details>
--
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]
