Convert stddev and stddev_pop to UDAF#10834
Conversation
github-actions
bot
added
logical-expr
| /// An accumulator to compute the average | ||
| #[derive(Debug)] |
There was a problem hiding this comment.
StddevAccumulator is used by correlation. That's why I keep it.
We should remove it after converting correlation to UDAF.
| } | ||
|
|
||
| #[test] | ||
| fn test_stddev_return_type() -> Result<()> { |
There was a problem hiding this comment.
I think those tests are covered by sqllogictests testing. I didn't add similar tests for UDAF.
| } | ||
|
|
||
| #[test] | ||
| fn test_stddev_expr() -> Result<()> { |
There was a problem hiding this comment.
Same as above. It's already covered by sqllogictests.
| } | ||
|
|
||
| #[test] | ||
| fn test_stddev_pop_expr() -> Result<()> { |
There was a problem hiding this comment.
Same as above. It's already covered by sqllogictests.
|
|
||
| fn accumulator(&self, acc_args: AccumulatorArgs) -> Result<Box<dyn Accumulator>> { | ||
| if acc_args.is_distinct { | ||
| return internal_err!("STDDEV_POP(DISTINCT) aggregations are not available"); |
There was a problem hiding this comment.
should it be not_impl_err?
| Ok(Box::new(StddevAccumulator::try_new(StatsType::Sample)?)) | ||
| } | ||
|
|
||
| fn create_sliding_accumulator( |
There was a problem hiding this comment.
I think we could rely on the default impl, since they are equivalent
There was a problem hiding this comment.
datafusion/datafusion/expr/src/udaf.rs
Lines 414 to 419 in 6b70214
| &self, | ||
| args: AccumulatorArgs, | ||
| ) -> Result<Box<dyn Accumulator>> { | ||
| self.accumulator(args) |
|
Thanks @goldmedal and @comphead |
|
Thanks again @jayzhan211 @comphead |
* add stddev and stddev_pot udaf * remove aggregation function stddev and stddev_pop * register func and modified return type * cargo fmt * regen proto * cargo clippy * fix window function support * cargo fmt * throw not_impl_err instead * use default sliding accumulator
3 participants
Which issue does this PR close?
Closes #10827 .
I converted
stddev_popto UDAF, too.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
no