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 09:01:09 UTC
[PR] fix: validate percentile bounds in calculate_percentile to prevent out-of-bounds panic and silent corruption [datafusion]
IgnatiusPang opened a new pull request, #25745:
URL: https://github.com/apache/datafusion/pull/25745
## Which issue does this PR close?
Closes #.
## Rationale for this change
In `datafusion-functions-aggregate::percentile_cont`, `calculate_percentile` computes the target index based on the input `percentile: f64`:
```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]));
}
--
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: validate percentile bounds in calculate_percentile to prevent out-of-bounds panic and silent corruption [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25745:
URL: https://github.com/apache/datafusion/pull/25745#issuecomment-6506000461
Thanks for the contribution! I am closing this under DataFusion's AI Dump policy.
https://datafusion.apache.org/contributor-guide/ai-policy.html#what-is-an-ai-dump-and-why-it-is-not-helpful
This has the hallmarks of AI code that was not reviewed by the author: a description that itself notes the changed code path is unreachable from SQL, tests that exercise only the pre-existing planner check, and no follow-up after this was one of eight PRs opened within a few hours.
If you would like to continue, please open a focused PR in your own words that you have reviewed and understand, and we are happy to help with review.
--
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: validate percentile bounds in calculate_percentile to prevent out-of-bounds panic and silent corruption [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #25745: fix: validate percentile bounds in calculate_percentile to prevent out-of-bounds panic and silent corruption
URL: https://github.com/apache/datafusion/pull/25745
--
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: validate percentile bounds in calculate_percentile to prevent out-of-bounds panic and silent corruption [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25745:
URL: https://github.com/apache/datafusion/pull/25745#issuecomment-5834302359
Marking as 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: validate percentile bounds in calculate_percentile to prevent out-of-bounds panic and silent corruption [datafusion]
Posted by "IgnatiusPang (via GitHub)" <gi...@apache.org>.
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: 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