[GitHub] [arrow-datafusion] Jimexist commented on a change in pull request #1762: [split/9+12] move udf, udaf, `Expr` to datafusion-expr module

[GitBox <gi...@apache.org>]

Jimexist commented on a change in pull request #1762:
URL: https://github.com/apache/arrow-datafusion/pull/1762#discussion_r802563462



##########
File path: datafusion/src/physical_plan/aggregates.rs
##########
@@ -40,15 +40,6 @@ use expressions::{
 };
 use std::sync::Arc;
 
-/// the implementation of an aggregate function

Review comment:
       > I like that you are slowly disentangling the physical from the logical plans
   > 
   > It seems like eventually we will have "physical plan" depending on Exprs and "logical plan" depending on exprs but not depending on each other 👍
   
   i had a look but i don't think it's happening anytime soon. now logical plan depends on table scan, which async scan to yield execution plan. it's hard to tear logican plan from execution plan given this chain.




-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org