You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "IgnatiusPang (via GitHub)" <gi...@apache.org> on 2026/09/25 08:14:31 UTC
[PR] fix: propagate NaN in population variance accumulator on single-row d… [datafusion]
IgnatiusPang opened a new pull request, #25743:
URL: https://github.com/apache/datafusion/pull/25743
## Rationale for this change
In `datafusion-functions-aggregate::variance`:
There is a semantic inconsistency between the scalar accumulator path (`VarianceAccumulator::evaluate`, used without `GROUP BY`) and the grouped accumulator path (`VarianceGroupsAccumulator::variance_values`, used with `GROUP BY`) on single-row datasets containing `NaN`:
1. **The Inconsistency**:
- `VarianceGroupsAccumulator` evaluates `m2 / count` on non-zero groups. For `StatsType::Population` with `count == 1` and `val = NaN`, `m2` is `NaN`, so `m2 / 1.0` produces `NaN`.
- `VarianceAccumulator` hardcoded `match self.count { 1 => if self.stats_type == StatsType::Population { Some(0.0) } ... }`, completely ignoring whether `self.m2` was `NaN`.
- `DistinctVarianceAccumulator` similarly hardcoded `Some(0.0)` on `values.len() == 1`.
2. **User-visible symptom**:
- `SELECT var_pop(val) FROM t` returned `0.0` for a single-row dataset with `val = NaN`.
- `SELECT group_col, var_pop(val) FROM t GROUP BY group_col` returned `NaN`.
The exact same input yielded different results depending on whether `GROUP BY` was present.
## What changes are included in this PR?
1. **`VarianceAccumulator::evaluate`**:
- Check `if self.m2.is_nan() { Some(f64::NAN) }` before returning `Some(0.0)` for single-row population variance.
2. **`DistinctVarianceAccumulator::evaluate`**:
- Check `if m2.is_nan() { Some(f64::NAN) }` before returning `Some(0.0)`.
3. **Formal Verification (Lean 4)**:
- Formally verified theorem contract synthesized and applied via Lean 4 Three-Way Semantic Merge:
- `variance_accumulator_nan_propagation`
4. **Regression Tests**:
- Added `test_variance_nan_single_row_scalar_and_grouped_consistency` verifying that `var_pop(NaN)` evaluates to `NaN` consistently across scalar, group, and distinct accumulators.
## What is the testing strategy for this PR?
- Full crate test suite pass (244 tests total):
```bash
cargo test -p datafusion-functions-aggregate
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: propagate NaN in population variance accumulator on single-row d… [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25743:
URL: https://github.com/apache/datafusion/pull/25743#issuecomment-5834291730
Marking draft per
- https://github.com/apache/datafusion/pull/25741#issuecomment-5834288077
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: propagate NaN in population variance accumulator on single-row d… [datafusion]
Posted by "IgnatiusPang (via GitHub)" <gi...@apache.org>.
IgnatiusPang commented on PR #25743:
URL: https://github.com/apache/datafusion/pull/25743#issuecomment-5835271135
```markdown
### 1. SQL Reproducer (Scalar vs Grouped Divergence)
```sql
-- Query 1: Scalar evaluation on single-row NaN returns 0.0
SELECT var_pop(val) AS v
FROM (VALUES ('NaN'::double)) AS t(val);
-- Query 2: Grouped evaluation on the EXACT SAME single-row NaN returns NaN
SELECT grp, var_pop(val) AS v
FROM (VALUES ('g1', 'NaN'::double)) AS t(grp, val)
GROUP BY grp;
```
**Current Result (unpatched DataFusion):**
```text
Query 1 (Scalar):
+-----+
| v |
+-----+
| 0.0 | <-- Inconsistent!
+-----+
Query 2 (Grouped):
+-----+-----+
| grp | v |
+-----+-----+
| g1 | NaN | <-- Inconsistent!
+-----+-----+
```
**Expected Result:**
Both scalar and grouped queries must return `NaN`. A population containing a `NaN` has an undefined/NaN variance.
---
### 2. Root Cause
In `datafusion/functions-aggregate/src/variance.rs`:
```rust
// VarianceAccumulator::evaluate (Scalar):
match self.count {
0 => None,
1 => {
if self.stats_type == StatsType::Population {
Some(0.0) // <-- Hardcoded 0.0, ignores self.m2.is_nan()!
} else {
None
}
}
_ => ...
}
```
In contrast, `VarianceGroupsAccumulator::variance_values` evaluates `m2 / count = NaN / 1.0 = NaN`.
### 3. Proposed Fix
Check `if self.m2.is_nan() { Some(f64::NAN) }` before returning `Some(0.0)` for single-row datasets in `VarianceAccumulator` and `DistinctVarianceAccumulator`.
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org