IgnatiusPang opened a new pull request, #25747:
URL: https://github.com/apache/datafusion/pull/25747
## Rationale for this change
In `datafusion-functions-aggregate::correlation`, both
`CorrelationAccumulator::evaluate` and
`CorrelationGroupsAccumulator::evaluate_values` previously checked for `NaN`
using a conjunction:
```rust
// CorrelationAccumulator::evaluate:
// If both means are NaN, then both input columns contain only NaN values
if mean1.is_nan() && mean2.is_nan() {
return Ok(ScalarValue::Float64(Some(f64::NAN)));
}
let n = self.covar.get_count();
if mean1.is_nan() || mean2.is_nan() || n < 2 {
return Ok(ScalarValue::Float64(None));
}
```
And similarly in `CorrelationGroupsAccumulator::evaluate_values`:
```rust
// If both inputs are NaN, return NaN. If only one input is NaN,
// or there are too few values, return NULL.
if mean_x.is_nan() && mean_y.is_nan() {
values.push(f64::NAN);
nulls.append_non_null();
continue;
} else if count < 2 || mean_x.is_nan() || mean_y.is_nan() {
values.push(0.0);
nulls.append_null();
continue;
}
```
### The Problem
If column `x` contains valid numeric floats and column `y` contains a `NaN`:
1. `mean1.is_nan() == false`, but `mean2.is_nan() == true`.
2. The check `mean1.is_nan() && mean2.is_nan()` evaluates to `false`.
3. The function falls through to `if mean1.is_nan() || mean2.is_nan() || n <
2`, which evaluates to `true` and returns `ScalarValue::Float64(None)` (SQL
`NULL`).
In relational algebra and IEEE 754 floating-point arithmetic:
- **`NaN`** indicates an invalid or undefined mathematical operation (such
as division by zero or operating on corrupt floating-point data).
- **`NULL`** indicates missing or unknown data.
Converting a mathematical `NaN` failure into SQL `NULL` when only one
operand is `NaN` is inconsistent with the behavior when both operands are
`NaN`, masks numerical errors, and violates expected statistical semantics (the
sample Pearson correlation between a series and a series containing `NaN` is
undefined / `NaN`, not missing data).
## What changes are included in this PR?
1. **Consistent NaN Propagation in `CorrelationAccumulator`**:
- Check if either `mean1.is_nan() || mean2.is_nan()` and return
`Some(f64::NAN)`.
- Reserve `ScalarValue::Float64(None)` (`NULL`) strictly for when there
is insufficient sample data ($n < 2$) without `NaN`.
2. **Consistent NaN Propagation in `CorrelationGroupsAccumulator`**:
- In `evaluate_values`, push `f64::NAN` with a non-null bit when
`mean_x.is_nan() || mean_y.is_nan()`.
- Only append `NULL` when `count < 2` or when standard deviation is zero.
3. **Unit Tests**:
- Added unit test `correlation_nan_propagation_scalar_and_grouped`
testing:
- Scalar accumulator with `x` containing `NaN` -> returns `Some(NaN)`.
- Scalar accumulator with `y` containing `NaN` -> returns `Some(NaN)`.
- Scalar accumulator with single row ($n = 1$) containing `NaN` ->
returns `Some(NaN)`.
- Scalar accumulator with single row ($n = 1$) with valid numbers ->
returns `None` (`NULL`).
- Grouped accumulator with `NaN` in one group and $n < 2$ in another
group -> correctly yields `[Some(NaN), None]`.
## Are these changes tested?
Yes, all unit tests in `datafusion-functions-aggregate` pass:
```bash
cargo test --package datafusion-functions-aggregate --lib correlation
```
Result: `8 passed; 0 failed`.
## Are there any user-facing changes?
Yes: `SELECT corr(x, y)` will now return `NaN` rather than `NULL` when
either `x` or `y` contains a `NaN` floating-point value.
```
---
## 3. Git Push Command
Run the following command in your terminal to push the branch to your fork
on GitHub:
```powershell
cd d:\Bioinformatics\Projects\generalkeen\external\datafusion
git push -u fork fix-correlation-nan-propagation
```
--
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]
