Replace supports_bounded_execution with supports_retract_batch #6695
Conversation
Rationale: The default implementation of the `Accumulator` trait returns an error for the `retract_batch` API.
github-actions
bot
added
core
# Conflicts: # datafusion/core/src/physical_plan/udaf.rs # datafusion/core/src/physical_plan/windows/mod.rs # datafusion/core/tests/user_defined_aggregates.rs
github-actions
bot
removed
logical-expr
| @@ -155,8 +155,7 @@ impl WindowExpr for PlainAggregateWindowExpr { | |||
| } | |||
|
|
|||
| fn uses_bounded_memory(&self) -> bool { | |||
| self.aggregate.supports_bounded_execution() | |||
| && !self.window_frame.end_bound.is_unbounded() | |||
| !self.window_frame.end_bound.is_unbounded() | |||
There was a problem hiding this comment.
After thinking through this logic. Actually, as long as end_bound is not bounded (not UNBOUNDED FOLLOWING such as in the form N FOLLOWING). We can produce results without waiting for the whole data to come (If accumulator do not support retract_batch method. We wouldn't be able to run queries in the form M PRECEDING and N FOLLOWING, in this case we will give an error anyway.). Hence here we do not need to check for self.aggregate.supports_bounded_execution() (acc.supports_retract_batch() method with the new API.)
| @@ -139,8 +139,7 @@ impl WindowExpr for SlidingAggregateWindowExpr { | |||
| } | |||
|
|
|||
| fn uses_bounded_memory(&self) -> bool { | |||
| self.aggregate.supports_bounded_execution() | |||
| && !self.window_frame.end_bound.is_unbounded() | |||
| !self.window_frame.end_bound.is_unbounded() | |||
There was a problem hiding this comment.
Similar to the case above.
| @@ -248,7 +248,7 @@ pub trait PartitionEvaluator: Debug + Send { | |||
| /// (3,4), | |||
| /// ] | |||
| /// ``` | |||
| fn evaluate_with_rank_all( | |||
| fn evaluate_all_with_rank( | |||
There was a problem hiding this comment.
I think, evaluate_all_with_rank is better name. As part of this PR, I changed the method from evaluate_with_rank_all to evaluate_all_with_rank.
| @@ -96,12 +96,6 @@ pub trait AggregateExpr: Send + Sync + Debug + PartialEq<dyn Any> { | |||
| false | |||
| } | |||
|
|
|||
| /// Specifies whether this aggregate function can run using bounded memory. | |||
There was a problem hiding this comment.
THis is nice to have moved this logic entirely to the accumulator 👍
Which issue does this PR close?
Related to #5781
Rationale for this change
With the changes in the #6671,
supports_retract_batchmethod is introduced to theAccumulatortrait. With the introduction ofsupports_retract_batchmethod,supports_bounded_executionmethod is no longer necessary for theAggregateExprtrait. (Similar to the case we have movesupports_bounded_executiontrait fromBuiltinWindowFunctionExprtoPartitionEvalautortrait.)This PR removes
supports_bounded_executionmethod fromAggregateExprand moves its functionality tosupports_retract_batchmethod in the Accumulator` trait for existing accumulators.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?