[GitHub] [arrow-datafusion] alamb commented on issue #2175: [Discuss] Different implementation style between Expr, LogicalPlan and ExecutionPlan

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

alamb commented on issue #2175:
URL: https://github.com/apache/arrow-datafusion/issues/2175#issuecomment-1092157017

   > And for physical ExecutionPlan, it is Trait/Trait Objects, I would prefer to use Enum also
   
   I agree this would be nice for code consistency. The fact it is a trait has always bothered me (mostly as I can't explain any good reason isn't the same as `LogicalPlan`. 
   
   I remember discussion in the past (maybe from @jorgecarleitao  and @andygrove?)  that using trait objects would allow for people to plug in their own physical plan / physical expr implementations. However, I haven't seen anyone try to do that recently (though I also haven't looked).
   
   > I think we should unify the coding style, at least the Expr and LogicalPlan representations should follow the same style.
   
   I think the rationale for extracting variants from `LogicalPlan` was that many of the enum variants had several fields and it was convenient in some places to pass a known type of LogicalPlan.
   
   I am not as convinced that adding an extra level of wrappers to `Expr` would make the code easier to work with
   
   For example, when I did out an example of what I think this proposal would lead to for `Expr::Not` and `Expr::Column` I got something like:
   
   ```rust
   struct Not(Expr);
   
   struct Column(schema::Column);
   
   enum Expr {
     ...
     Column(Column),
     Not(Not)
     ...
   }
   
   ```
   
   And I think it makes the code significantly *harder* to read and work with (as now `.0` would get sprinkled around)
   
   If we feel a deep need for consistency, I would personally prefer to expand `LogicalPlan` back to direct variants


-- 
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