You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kumarUjjawal (via GitHub)" <gi...@apache.org> on 2026/02/07 12:21:34 UTC
[PR] fix: percentile_cont interpolation causes NaN for f16 input [datafusion]
kumarUjjawal opened a new pull request, #20208:
URL: https://github.com/apache/datafusion/pull/20208
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #18945
## Rationale for this change
percentile_cont interpolation for Float16 could overflow f16 intermediates (e.g. when scaling the fractional component), producing inf/NaN and incorrect results. This PR makes interpolation numerically safe for f16.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
• Perform percentile interpolation in f64 and cast back to the input float type (f16/f32/f64) to avoid f16 overflow.
• Add a regression unit test covering Float16 interpolation near the maximum finite value.
• Remove the unused interpolation precision constant and associated wrapping-math code paths/comments.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
Yes. percentile_cont on Float16 inputs no longer returns NaN due to interpolation overflow and will produce correct
finite results for valid finite f16 data
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: percentile_cont interpolation causes NaN for f16 input [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
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: 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: percentile_cont interpolation causes NaN for f16 input [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20208:
URL: https://github.com/apache/datafusion/pull/20208#discussion_r2787966144
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -58,16 +58,10 @@ use datafusion_macros::user_doc;
use crate::utils::validate_percentile_expr;
-/// Precision multiplier for linear interpolation calculations.
-///
-/// This value of 1,000,000 was chosen to balance precision with overflow safety:
-/// - Provides 6 decimal places of precision for the fractional component
-/// - Small enough to avoid overflow when multiplied with typical numeric values
-/// - Sufficient precision for most statistical applications
-///
-/// The interpolation formula: `lower + (upper - lower) * fraction`
-/// is computed as: `lower + ((upper - lower) * (fraction * PRECISION)) / PRECISION`
-/// to avoid floating-point operations on integer types while maintaining precision.
+// Keep historical behavior for percentile interpolation by quantizing the
+// fractional component before interpolation. This minimizes output changes for
+// Float32/Float64 while still allowing us to perform the arithmetic in f64
+// (preventing Float16 overflow).
Review Comment:
Can we keep the original comment here? I don't see why we should replace the proper explanation with simply "Keep historical behaviour"
##########
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:
Why are we removing the wrapping versions of the operators?
##########
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
Review Comment:
Same here
--
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: percentile_cont interpolation causes NaN for f16 input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20208:
URL: https://github.com/apache/datafusion/pull/20208#issuecomment-3898295406
Thanks @kumarUjjawal and @Jefffrey
--
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: percentile_cont interpolation causes NaN for f16 input [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20208:
URL: https://github.com/apache/datafusion/pull/20208#discussion_r2796462050
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -772,22 +787,49 @@ 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.
+ //
+ // We quantize the fractional component (via `INTERPOLATION_PRECISION`) to
+ // minimize output changes for Float32/Float64 compared to the previous
Review Comment:
What previous implementation are we referring to here?
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -772,22 +787,49 @@ 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.
+ //
+ // We quantize the fractional component (via `INTERPOLATION_PRECISION`) to
+ // minimize output changes for Float32/Float64 compared to the previous
+ // implementation.
+ //
+ // We perform the arithmetic in f64 and then cast back to the native type to
+ // avoid overflowing Float16 intermediates.
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:
If we always cast `INTERPOLATION_PRECISION` to `f64` at all its usages we should just define it as `f64`
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -58,16 +58,14 @@ use datafusion_macros::user_doc;
use crate::utils::validate_percentile_expr;
-/// Precision multiplier for linear interpolation calculations.
+/// Precision multiplier used to quantize the fractional component of the
+/// interpolation weight.
///
-/// This value of 1,000,000 was chosen to balance precision with overflow safety:
-/// - Provides 6 decimal places of precision for the fractional component
-/// - Small enough to avoid overflow when multiplied with typical numeric values
-/// - Sufficient precision for most statistical applications
+/// We keep this quantization to minimize output changes for Float32/Float64
+/// compared to the previous implementation.
///
-/// The interpolation formula: `lower + (upper - lower) * fraction`
-/// is computed as: `lower + ((upper - lower) * (fraction * PRECISION)) / PRECISION`
-/// to avoid floating-point operations on integer types while maintaining precision.
+/// Interpolation is performed in f64 and then cast back to the native type to
+/// avoid overflowing Float16 intermediates.
Review Comment:
I'd still prefer we keep the old documentation as it was and just add on any amendments; I don't like how it reads about a "previous implementation" considering there is no information about such implementation (without looking at the git history)
--
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: percentile_cont interpolation causes NaN for f16 input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #20208:
URL: https://github.com/apache/datafusion/pull/20208
--
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: percentile_cont interpolation causes NaN for f16 input [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20208:
URL: https://github.com/apache/datafusion/pull/20208#discussion_r2778423798
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
@@ -58,17 +58,50 @@ use datafusion_macros::user_doc;
use crate::utils::validate_percentile_expr;
-/// Precision multiplier for linear interpolation calculations.
-///
-/// This value of 1,000,000 was chosen to balance precision with overflow safety:
-/// - Provides 6 decimal places of precision for the fractional component
-/// - Small enough to avoid overflow when multiplied with typical numeric values
-/// - Sufficient precision for most statistical applications
-///
-/// The interpolation formula: `lower + (upper - lower) * fraction`
-/// is computed as: `lower + ((upper - lower) * (fraction * PRECISION)) / PRECISION`
-/// to avoid floating-point operations on integer types while maintaining precision.
-const INTERPOLATION_PRECISION: usize = 1_000_000;
+/// Helper trait for safe interpolation on floating point native types
+/// (Float16 / Float32 / Float64).
+trait InterpolateNative: Copy {
Review Comment:
I feel there should be existing traits we can leverage for this
e.g.
- https://docs.rs/half/latest/half/struct.f16.html#impl-AsPrimitive%3Cf64%3E-for-f16
- https://docs.rs/half/latest/half/struct.f16.html#impl-AsPrimitive%3Cf16%3E-for-f64
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3926,7 +3926,7 @@ SELECT percentile_cont(0.95) WITHIN GROUP (ORDER BY c3 DESC) FROM aggregate_test
query R
SELECT percentile_cont(0.05) WITHIN GROUP (ORDER BY c3 DESC) FROM aggregate_test_100
----
-118.099998
+118.1
Review Comment:
Not sure if this precision loss for other float types is desirable as part of these changes
--
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