Remove special casting of Min / Max built in AggregateFunctions

[alamb]

Is your feature request related to a problem or challenge?

Part of #10943

While trying to port min and max to UDFs in #11013, @edmondop found several places where Min and Max (the existing built in aggregate functions) are special cased

Here is Max:

datafusion/datafusion/physical-expr/src/aggregate/min_max.rs

Lines 77 to 82 in c2ea6b3

	pub struct Max {
	name: String,
	data_type: DataType,
	nullable: bool,
	expr: Arc<dyn PhysicalExpr>,
	}

The problem with relying on Min and Max directly is that it is hard/impossible to switch Min/Max to be a user defined aggregates as seen in #11013

Also, relying on Min/Max directly means that there is certain behavior that is not available to UDAFs compared to build in aggregate functions, which isn't ideal

Describe the solution you'd like

Remove all explicit references to Min / Max

Specifically, code that looks like this should be removed

       expr.as_any().downcast_ref::<Max>()

and

       expr.as_any().downcast_ref::<Min>()

Describe alternatives you've considered

Ideally, the alternative is to use a function on AggregateExpr which we can then add/implement for UDAFs when we port min/max to be a UDAF

Task List

• Remove Min/Max references from AggregateExec::get_minmax_descr #11152
• Remove Min/Max references from AggregateStatistics #11153

Additional context

No response

[edmondop]

Do you think we should implement a special is_min/is_max since these are pervasive and used also for statistics ?

[alamb]

Do you think we should implement a special is_min/is_max since these are pervasive and used also for statistics ?

It might make sense to special case is_min and is_max

What we did in the case of ScalarUDFs was to go with functions that described the property in question was rather than what the function was. So for example, for coalesce and some array functions which needed special case type coercion rules, @jayzhan211 added a way to provide user defined coercion rules, rather than a is_coalesce function

I was thinking we would do something similar in this case. However, it if turns out that all such properties are only relevant for min/max maybe having a is_min / is_max API would be better than enumerating all the properties

[edmondop]

@alamb I think this has been solved?

[jayzhan211]

This is not, we need to address the specialization check in aggregate_statistic

[alamb]

Specifically I think these checks need to be removed:

datafusion/datafusion/physical-optimizer/src/aggregate_statistics.rs

Lines 269 to 292 in 4838cfb

	fn is_non_distinct_count(agg_expr: &AggregateFunctionExpr) -> bool {
	if agg_expr.fun().name() == "count" && !agg_expr.is_distinct() {
	return true;
	}
	false
	}
	// TODO: Move this check into AggregateUDFImpl
	// https://github.com/apache/datafusion/issues/11153
	fn is_min(agg_expr: &AggregateFunctionExpr) -> bool {
	if agg_expr.fun().name().to_lowercase() == "min" {
	return true;
	}
	false
	}
	// TODO: Move this check into AggregateUDFImpl
	// https://github.com/apache/datafusion/issues/11153
	fn is_max(agg_expr: &AggregateFunctionExpr) -> bool {
	if agg_expr.fun().name().to_lowercase() == "max" {
	return true;
	}
	false
	}

[alamb]

Here is one specific suggestion: #12296 (comment)