feat: widen percentile input to use larger precision - #24577
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24577 +/- ##
==========================================
+ Coverage 81.37% 81.48% +0.10%
==========================================
Files 1116 1122 +6
Lines 397509 404264 +6755
Branches 397509 404264 +6755
==========================================
+ Hits 323461 329398 +5937
- Misses 55110 55551 +441
- Partials 18938 19315 +377 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@Jefffrey could you have a look please? A follow-up for exact percentiles |
kosiew
left a comment
There was a problem hiding this comment.
@theirix, thanks for working on this. The change looks good to me. Using the shared interpolation precision and widening the bounded remainder arithmetic to i64 addresses the Decimal32 issue cleanly. I just have one non-blocking suggestion around coverage for the overflow fallback path.
|
|
||
| // Happy path: `upper - lower` does not overflow | ||
| // (could be a case for Decimal128 with max precision) | ||
| if let Ok(delta) = upper.sub_checked(lower) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Thank you, it's a good idea - added a specialised test.
|
🚀 |
Which issue does this PR close?
Decimal32with lower interpolation precision #24576Rationale for this change
A follow-up for #24419, which introduced pure-decimal interpolation, where a smaller interpolation precision was used for
Decimal32types with Euclidean interpolation. Seems like, a smaller precision forDecimal32is not enough, and the value was saturated to zero.Solution - just widen
i32inputs toi64to handle edge cases. In this case, we can use the same logical interpolation precision10^6everywhere.What changes are included in this PR?
scale_by_numnow widensi32inputs toi64Are these changes tested?
Are there any user-facing changes?