[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6671: Allow `AggregateUDF` to define retractable batch , efficiently implement window functions

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #6671:
URL: https://github.com/apache/arrow-datafusion/pull/6671#discussion_r1230803124


##########
datafusion/core/src/physical_plan/udaf.rs:
##########
@@ -70,6 +70,11 @@ impl AggregateFunctionExpr {
     pub fn fun(&self) -> &AggregateUDF {
         &self.fun
     }
+
+    /// Returns true if this can support sliding accumulators
+    pub fn retractable(&self) -> Result<bool> {
+        Ok((self.fun.accumulator)(&self.data_type)?.supports_retract_batch())

Review Comment:
   Another other formulation I tried looked like the following. The upside is it avoided the instantiation, but the downside is that it an API change and is not consistent with the window functions
   
   ```rust
   pub struct AggregateUDF {
       /// name
       pub name: String,
       /// Signature (input arguments)
       pub signature: Signature,
       /// Return type
       pub return_type: ReturnTypeFunction,
       /// Return an accumulator without retract batch
       pub accumulator: AccumulatorFunctionImplementation,
       /// Return an accumulator with retract batch
       pub retractable_accumulator: Option<AccumulatorFunctionImplementation>,  <---- this is added
       /// the accumulator's state's description as a function of the return type
       pub state_type: StateTypeFunction,
   }
   ```



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org