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

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

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

   I agree with @alamb that if we want to go for consistency then it would be better to make `LogicalPlan` a trait rather than to make `ExecutionPlan` and enum. Since this is a potential extension point it seems a little awkward to model it as a sum type where we have add an additional layer of indirection for defining user-defined physical/logical plan nodes. In our project, we do create a custom `UserDefinedLogicalNode` and an associated custom `ExecutionPlan` and it feels much more ergonomic to create the custom `ExecutionPlan` than it does to create the `UserDefinedLogicalNode`. 
   
   That said, in things like serde logic where we need to handle all possible variants it is nicer (and will be even nicer still once `deref patterns` lands :)) to be able to pattern match all the variants rather than do the long chain of "if downcast_ref" conditions, so I don't feel strongly either way. 


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