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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
