kosiew commented on code in PR #24577:
URL: https://github.com/apache/datafusion/pull/24577#discussion_r3878343295
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -994,18 +1002,15 @@ where
debug_assert!((0.0..=1.0).contains(&fraction));
debug_assert!(lower <= upper);
- let interpolation_precision = deduce_interpolation_precision::<T>();
-
- let num =
- T::Native::usize_as((fraction * interpolation_precision as f64) as
usize);
- let den = T::Native::usize_as(interpolation_precision);
+ let num = (fraction * INTERPOLATION_PRECISION as f64) as i64;
+ let den = INTERPOLATION_PRECISION as i64;
// Happy path: `upper - lower` does not overflow
// (could be a case for Decimal128 with max precision)
if let Ok(delta) = upper.sub_checked(lower) {
Review Comment:
Could we add a focused unit test that exercises the
`upper.sub_checked(lower)` overflow fallback? For example, Decimal32 with
`i32::MIN` and `i32::MAX` at a non-endpoint fraction should force this path.
The new SQL tests cover the normal delta path nicely, but a test like this
would also lock down the separately weighted extrema path that relies on the
widened remainder arithmetic.
--
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]
