feat: decimal support for percentile_cont and median - #24419
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24419 +/- ##
==========================================
+ Coverage 81.23% 81.25% +0.01%
==========================================
Files 1111 1112 +1
Lines 390208 390464 +256
Branches 390208 390464 +256
==========================================
+ Hits 316990 317257 +267
+ Misses 54591 54556 -35
- Partials 18627 18651 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CC @neilconway |
Jefffrey
left a comment
There was a problem hiding this comment.
Integer support for
percentile_cont(backported frommedianUDF)
this doesnt sound right, since this is continuous and not discrete so it must always be a float (or decimal)
| #[derive(PartialEq, Eq, Hash, Debug)] | ||
| pub struct Median { | ||
| signature: Signature, | ||
| percentile_cont: PercentileCont, |
There was a problem hiding this comment.
i do wonder if we should implement simplify for median to make it an unconditional rewrite to percentile_cont
| Coercion::new_implicit( | ||
| TypeSignatureClass::Native(logical_float64()), | ||
| vec![TypeSignatureClass::Numeric], | ||
| NativeType::Float64, | ||
| ), |
There was a problem hiding this comment.
| Coercion::new_implicit( | |
| TypeSignatureClass::Native(logical_float64()), | |
| vec![TypeSignatureClass::Numeric], | |
| NativeType::Float64, | |
| ), | |
| Coercion::new_implicit_native( | |
| logical_float64(), | |
| vec![TypeSignatureClass::Numeric], | |
| ), |
- new simpler API from feat: add implicit native coercion constructor #24282
| } | ||
|
|
||
| fn accumulator(&self, args: AccumulatorArgs) -> Result<Box<dyn Accumulator>> { | ||
| let percentile = get_percentile(&args)?; |
There was a problem hiding this comment.
i think its intentional to check the percentile up here, so that we still error on invalid percentiles even if input is null
| } | ||
|
|
||
| downcast_integer! { | ||
| input_dt => (integer_helper, input_dt), |
There was a problem hiding this comment.
are integer types supported in the signature? is this code being reached?
Which issue does this PR close?
medianandpercentile_contimplementations (preserve Decimal inpercentile_cont, aliasmedian) #22042.Rationale for this change
UDF
percentile_contuses floating-point arithmetic to find a percentile, which doesn't work well for decimals. ThemedianUDF already supports decimals. Let's implement decimals inpercentile_contand buildmedianon top of it.A follow-up to #21988, refines #23954.
What changes are included in this PR?
percentile_cont, without float coercionpercentile_cont(backported frommedianUDF)medianon top ofpercentile_contUDF by prepending a0.5fargumentpercentile_cont(NULL)return NULL (see changes below), under a new SLT test caseAre these changes tested?
percentile_contAre there any user-facing changes?
percentile_cont(NULL)now returns a NULL type instead of a Float64 type, aligning with other aggregate UDFs