Remove AggregateExpr abstraction layer

[lewiszlw]

Is your feature request related to a problem or challenge?

Now all aggr functions have been migrated to UDAF. The AggregateExpr trait just has one implementation.

impl AggregateExpr for AggregateFunctionExpr {
    ...
}

Should we remove unnecessary AggregateExpr abstraction layer?

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

[jayzhan211]

It is not clear to me whether it is a good idea. Maybe hold on and review it after the refactor #11359 ?

[lewiszlw]

Sure. We could wait to see more opinions.

[alamb]

Now all aggr functions have been migrated to UDAF. The AggregateExpr trait just has one implementation.

This seems like a compelling reason to remove AggregateExpr (it is unecessary and makes the code more complicated)

It is not clear to me whether it is a good idea. Maybe hold on and review it after the refactor #11359 ?

Now that #11359 is done, @jayzhan211 do you have any additional thoughts?

I agree with your comment on https://github.com/apache/datafusion/pull/11989/files#r1720745280 that there is room for improvement here

[jayzhan211]

I think we could try removing trait AggregateExpr and see whether this simplified the code.

[lewiszlw]

trait AggregateExpr and AggregateFunctionExpr live in different crates. We should solve dependency problem first...

[jayzhan211]

I think the code that has AggregateExpr are all in physical layer, replacing AggregateExpr with AggregateFunctionExpr doesn't have dependency issue, did I miss anything?