alamb commented on code in PR #24728:
URL: https://github.com/apache/datafusion/pull/24728#discussion_r3873346597
##########
datafusion/expr/src/udaf.rs:
##########
@@ -943,258 +955,498 @@ impl PartialOrd for dyn AggregateUDFImpl {
}
}
+/// Builds the default implementation of [`AggregateUDFImpl::schema_name`].
Review Comment:
Here is a new builder for schema name -- it is basically the same pattern
for all of the other types
##########
datafusion/expr/src/udaf.rs:
##########
@@ -459,15 +459,21 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash +
Send + Sync + Any {
/// See [`Expr::schema_name`] for details
///
/// Example of schema_name: count(DISTINCT column1) FILTER (WHERE column2
> 10) ORDER BY [..]
+ ///
+ /// The default implementation is provided by [`UdafSchemaNameBuilder`]
fn schema_name(&self, params: &AggregateFunctionParams) -> Result<String> {
- udaf_default_schema_name(self, params)
+ UdafSchemaNameBuilder::new(self.name(), params)
Review Comment:
The core rationale is to use a single non generic function -- I also made
a struct like builder to make the code easier to see too
##########
datafusion/expr/src/udaf.rs:
##########
@@ -943,258 +955,498 @@ impl PartialOrd for dyn AggregateUDFImpl {
}
}
+/// Builds the default implementation of [`AggregateUDFImpl::schema_name`].
+///
+/// # Example
+/// ```
+/// # use datafusion_expr::col;
+/// # use datafusion_expr::expr::{AggregateFunctionParams, Sort};
+/// # use datafusion_expr::UdafSchemaNameBuilder;
+/// # let params = AggregateFunctionParams {
+/// # args: vec![col("x")],
+/// # distinct: true,
+/// # filter: None,
+/// # order_by: vec![Sort::new(col("y"), true, false)],
+/// # null_treatment: None,
+/// # };
+/// let schema_name = UdafSchemaNameBuilder::new("my_udaf", ¶ms)
+/// .with_supports_within_group_clause(false)
+/// .build()
+/// .unwrap();
+/// assert_eq!(schema_name, "my_udaf(DISTINCT x) ORDER BY [y ASC NULLS LAST]");
+/// ```
+#[derive(Debug)]
+pub struct UdafSchemaNameBuilder<'a> {
+ name: &'a str,
+ params: &'a AggregateFunctionParams,
+ supports_within_group_clause: bool,
+}
+
+impl<'a> UdafSchemaNameBuilder<'a> {
+ /// Create a new builder for the function named `name`
+ /// (the result of [`AggregateUDFImpl::name`])
+ pub fn new(name: &'a str, params: &'a AggregateFunctionParams) -> Self {
+ Self {
+ name,
+ params,
+ supports_within_group_clause: false,
+ }
+ }
+
+ /// Set whether the function supports the `WITHIN GROUP` clause
+ /// (the result of [`AggregateUDFImpl::supports_within_group_clause`]).
+ /// Defaults to `false`.
+ pub fn with_supports_within_group_clause(
+ mut self,
+ supports_within_group_clause: bool,
+ ) -> Self {
+ self.supports_within_group_clause = supports_within_group_clause;
+ self
+ }
+
+ /// Build the schema name
+ pub fn build(self) -> Result<String> {
+ let Self {
+ name,
+ params,
+ supports_within_group_clause,
+ } = self;
+
+ let AggregateFunctionParams {
+ args,
+ distinct,
+ filter,
+ order_by,
+ null_treatment,
+ } = params;
+
+ // exclude the first function argument(= column) in ordered set
aggregate function,
Review Comment:
this logic is all the same -- there is just now some more builder ceremony
logic
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
