IgnatiusPang commented on PR #25745:
URL: https://github.com/apache/datafusion/pull/25745#issuecomment-5835288831
```markdown
### 1. Reproducer & Defect Analysis
While standard SQL literal percentiles are validated during physical
planning by `validate_percentile_expr`, direct callers of
`calculate_percentile` or accumulators constructed programmatically hit two
severe defects:
1. **Silent Data Corruption on Negative or NaN Percentiles**:
```rust
let index = percentile * ((len - 1) as f64);
let lower_index = index.floor() as usize;
let upper_index = index.ceil() as usize;
if lower_index == upper_index {
values.select_nth_unstable_by(lower_index, cmp);
return Ok(Some(values[lower_index]));
}
```
When `percentile < 0.0` (e.g. `-0.5`) or `percentile.is_nan()`:
- In Rust, casting a negative float to `usize` saturates to `0`.
- `lower_index == upper_index == 0` evaluates to `true`.
- The function silently returns `values[0]` (the minimum value) rather
than returning an execution error.
2. **Runtime Panic on Percentiles Exceeding 1.0**:
When `percentile > 1.0` (e.g. `1.5`):
- `lower_index` and `upper_index` exceed `len - 1`.
- Calling `values.select_nth_unstable_by(lower_index, cmp)` panics:
`index out of bounds: the len is ... but the index is ...`
---
### 2. Proposed Fix
Add finite and interval validation directly inside `calculate_percentile`:
```rust
if !percentile.is_finite() || !(0.0..=1.0).contains(&percentile) {
return datafusion_common::exec_err!(
"Percentile value must be between 0.0 and 1.0 inclusive, got
{percentile}"
);
}
```
```
--
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]
