alamb commented on code in PR #10651:
URL: https://github.com/apache/datafusion/pull/10651#discussion_r1621402356
##########
datafusion/functions-aggregate/src/lib.rs:
##########
@@ -110,6 +113,11 @@ mod tests {
fn test_no_duplicate_name() -> Result<()> {
let mut names = HashSet::new();
for func in all_default_aggregate_functions() {
+ // TODO: remove this
+ // sum is in intermidiate migration state, skip this
Review Comment:
can you add a ticket reference to the issue that completes the migration?
##########
datafusion/core/tests/user_defined/user_defined_aggregates.rs:
##########
@@ -142,7 +142,7 @@ async fn
test_udaf_as_window_with_frame_without_retract_batch() {
let sql = "SELECT time_sum(time) OVER(ORDER BY time ROWS BETWEEN 1
PRECEDING AND 1 FOLLOWING) as time_sum from t";
// Note if this query ever does start working
let err = execute(&ctx, sql).await.unwrap_err();
- assert_contains!(err.to_string(), "This feature is not implemented:
Aggregate can not be used as a sliding accumulator because `retract_batch` is
not implemented: AggregateUDF { inner: AggregateUDF { name: \"time_sum\",
signature: Signature { type_signature: Exact([Timestamp(Nanosecond, None)]),
volatility: Immutable }, fun: \"<FUNC>\" } }(t.time) ORDER BY [t.time ASC NULLS
LAST] ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING");
+ assert_contains!(err.to_string(), "This feature is not implemented:
Aggregate can not be used as a sliding accumulator because `retract_batch` is
not implemented: time_sum(t.time) ORDER BY [t.time ASC NULLS LAST] ROWS BETWEEN
1 PRECEDING AND 1 FOLLOWING");
Review Comment:
that is certainly nicer
##########
datafusion/expr/src/udaf.rs:
##########
@@ -389,6 +410,13 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
&[]
}
+ fn create_sliding_accumulator(
Review Comment:
Could we please add some documentation here explaining what this is?
Basically I think this is to allow returning a different [`Accumulator`
](https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.Accumulator.html)instance
that is optimized for sliding windows (e.g. incrementally computing output via
https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.Accumulator.html#method.retract_batch
##########
datafusion/expr/src/udaf.rs:
##########
@@ -459,7 +515,7 @@ pub enum ReversedUDAF {
/// The expression does not support reverse calculation, like ArrayAgg
NotSupported,
/// The expression is different from the original expression
- Reversed(Arc<dyn AggregateUDFImpl>),
+ Reversed(Arc<AggregateUDF>),
Review Comment:
👍
##########
datafusion/expr/src/udaf.rs:
##########
@@ -451,6 +479,34 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
fn reverse_expr(&self) -> ReversedUDAF {
ReversedUDAF::NotSupported
}
+
+ /// Coerce arguments of a function call to types that the function can
evaluate.
+ ///
+ /// This function is only called if [`AggregateUDFImpl::signature`]
returns [`crate::TypeSignature::UserDefined`]. Most
Review Comment:
👍
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
