kumarUjjawal commented on code in PR #20208:
URL: https://github.com/apache/datafusion/pull/20208#discussion_r2788456782
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -772,22 +783,42 @@ fn calculate_percentile<T: ArrowNumericType>(
let (_, upper_value, _) =
values.select_nth_unstable_by(upper_index, cmp);
let upper_value = *upper_value;
- // Linear interpolation using wrapping arithmetic
- // We use wrapping operations here (matching the approach in
median.rs) because:
- // 1. Both values come from the input data, so diff is bounded by
the value range
- // 2. fraction is between 0 and 1, and INTERPOLATION_PRECISION is
small enough
- // to prevent overflow when combined with typical numeric ranges
- // 3. The result is guaranteed to be between lower_value and
upper_value
- // 4. For floating-point types, wrapping ops behave the same as
standard ops
+ // Linear interpolation (see `INTERPOLATION_PRECISION` comment for
rationale).
let fraction = index - (lower_index as f64);
- let diff = upper_value.sub_wrapping(lower_value);
- let interpolated = lower_value.add_wrapping(
- diff.mul_wrapping(T::Native::usize_as(
- (fraction * INTERPOLATION_PRECISION as f64) as usize,
- ))
- .div_wrapping(T::Native::usize_as(INTERPOLATION_PRECISION)),
- );
- Some(interpolated)
+ let scaled = (fraction * INTERPOLATION_PRECISION as f64) as usize;
Review Comment:
moved the interpolation math to f64 (then casts back), so there’s no
native-type overflow to wrap anymore.
--
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]
