Move create_physical_expr to phy-expr-common #3

[jayzhan211]

Which issue does this PR close?

Closes #10176 Closes #10074 Closes #10144

All in one in this PR.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[alamb]

Hi @jayzhan211 -- if we merge this one, does it close https://github.com/apache/datafusion/pull/10176 and https://github.com/apache/datafusion/pull/10144 ?

[jayzhan211]

Hi @jayzhan211 -- if we merge this one, does it close #10176 and #10144 ?

Yes!

[alamb]

Sorry for being dense -- would you like me to review this PR? Or do you think we should review (and merge) https://github.com/apache/datafusion/pull/10176 and https://github.com/apache/datafusion/pull/10144 first?

[jayzhan211]

It depends on you and other reviewers, If you don't mind the large PR, definitely it is nice we can merge this one. Otherwise, we can close #10144 first, and move on to #10176 and finally this one.

I didn't merge #10144 because I'm also curious about what is it like at the final PR, make sure not all the things are moved to common

[alamb]

Here is another suggestion: https://github.com/apache/datafusion/issues/10074#issuecomment-2077439515

[jayzhan211]

Once we have created a Arc<dyn PhysicalExpr> I don't really understand how/why we would ever get an Expr that we need to convert back to Arc<dyn PhysicalExpr> again.

I agree, but it turns out I didn't find a way to reuse ordering_req in AggregateFunctionExpr, because we can't have physical-expr in AggregateUDFImpl, so is not able to get it straightaway

End state of dependencies??

I agree with the graph, which is in my mind too.

[jayzhan211]

If we can move reverse_expr related logic all to logical layer.... 🤔