alamb commented on code in PR #10525:
URL: https://github.com/apache/datafusion/pull/10525#discussion_r1602179481


##########
datafusion/expr/src/function.rs:
##########
@@ -66,22 +69,39 @@ pub struct AccumulatorArgs<'a> {
     ///
     /// If no `ORDER BY` is specified, `sort_exprs`` will be empty.
     pub sort_exprs: &'a [Expr],
+
+    /// Whether the aggregate function is distinct.
+    pub is_distinct: bool,
+
+    /// The input type of the aggregate function.
+    pub input_type: &'a DataType,
+}
+
+/// [`GroupsAccumulatorSupportedArgs`] contains information to determine if an
+/// aggregate function supports the groups accumulator.
+pub struct GroupsAccumulatorSupportedArgs {

Review Comment:
   I know the current implementation of `groups_accumulator_supported` only 
needs these two fields, but I can see in the future needing more of them
   
   Is there any reason we can't just pass `AccumulatorArgs` to 
`groups_accumulator_supported`?



##########
datafusion/expr/src/function.rs:
##########
@@ -66,22 +69,39 @@ pub struct AccumulatorArgs<'a> {
     ///
     /// If no `ORDER BY` is specified, `sort_exprs`` will be empty.
     pub sort_exprs: &'a [Expr],
+
+    /// Whether the aggregate function is distinct.
+    pub is_distinct: bool,
+
+    /// The input type of the aggregate function.
+    pub input_type: &'a DataType,
+}
+
+/// [`GroupsAccumulatorSupportedArgs`] contains information to determine if an
+/// aggregate function supports the groups accumulator.
+pub struct GroupsAccumulatorSupportedArgs {
+    /// The number of arguments the aggregate function takes.
+    pub args_num: usize,
+
+    /// Whether the aggregate function is distinct.
+    pub is_distinct: bool,
 }
 
-impl<'a> AccumulatorArgs<'a> {
-    pub fn new(
-        data_type: &'a DataType,
-        schema: &'a Schema,
-        ignore_nulls: bool,
-        sort_exprs: &'a [Expr],
-    ) -> Self {
-        Self {
-            data_type,
-            schema,
-            ignore_nulls,
-            sort_exprs,
-        }
-    }
+pub struct StateFieldsArgs<'a> {

Review Comment:
   Can we please add doc comments to this explaining what it is for?



-- 
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

Reply via email to