findepi commented on code in PR #14689:
URL: https://github.com/apache/datafusion/pull/14689#discussion_r1961749786
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -139,6 +148,185 @@ impl AggregateUDFImpl for Count {
"count"
}
+ fn schema_name(&self, params: &AggregateFunctionParams) -> Result<String> {
+ let AggregateFunctionParams {
+ args,
+ distinct,
+ filter,
+ order_by,
+ null_treatment,
+ } = params;
+
+ let mut schema_name = String::new();
+
+ if !args.is_empty() && args[0] == Expr::Literal(COUNT_STAR_EXPANSION) {
+ schema_name.write_str("count(*)")?;
+ } else {
+ schema_name.write_fmt(format_args!(
+ "{}({}{})",
+ self.name(),
+ if *distinct { "DISTINCT " } else { "" },
+ schema_name_from_exprs(args)?
+ ))?;
+ }
+
+ if let Some(null_treatment) = null_treatment {
+ schema_name.write_fmt(format_args!(" {}", null_treatment))?;
+ }
+
+ if let Some(filter) = filter {
+ schema_name.write_fmt(format_args!(" FILTER (WHERE {filter})"))?;
+ };
+
+ if let Some(order_by) = order_by {
+ schema_name.write_fmt(format_args!(
+ " ORDER BY [{}]",
+ schema_name_from_sorts(order_by)?
+ ))?;
+ };
+
+ Ok(schema_name)
+ }
Review Comment:
This looks like a long copy of the default implementation.
Overall we have 4 methods copied, 177 lines overall, where all we need is
customize that count(1) is displayed as count(*). Not good for maintainability.
I wonder why the logic for formatting distinct, filter and order by is
handed to the function itself, if it's attribute of the containing
AggregateFunction. If we want to solve this, this could be a prep PR to avoid
PR scope screep.
Otherwise it's better to leave count(1) as count(1), rather than copy so
many lines, unless some other option exits.
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -139,6 +148,185 @@ impl AggregateUDFImpl for Count {
"count"
}
+ fn schema_name(&self, params: &AggregateFunctionParams) -> Result<String> {
+ let AggregateFunctionParams {
+ args,
+ distinct,
+ filter,
+ order_by,
+ null_treatment,
+ } = params;
+
+ let mut schema_name = String::new();
+
+ if !args.is_empty() && args[0] == Expr::Literal(COUNT_STAR_EXPANSION) {
+ schema_name.write_str("count(*)")?;
Review Comment:
- `count(1)` and `count(2)` are the same thing, so what about checking for
args[0] to be a non-null constant?
- `count(1, a, b)` is something else than `count(1)`; this should check that
args.len = 1
##########
datafusion/expr/src/planner.rs:
##########
@@ -167,14 +170,14 @@ pub trait ExprPlanner: Debug + Send + Sync {
/// Plan an extract expression, such as`EXTRACT(month FROM foo)`
///
- /// Returns origin expression arguments if not possible
+ /// Returns original expression arguments if not possible
Review Comment:
This is a good change. nit Could go in separate PR to keep PR size lower.
--
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
