tustvold commented on issue #2175:
URL:
https://github.com/apache/arrow-datafusion/issues/2175#issuecomment-1094950365
> with_new_children_if_necessary
FWIW you could use an associated trait function (or a free function as the
PR does)
```
fn with_new_children_if_necessary(s: Arc<dyn ExecutionPlan>, children:
Vec<Arc<dyn ExecutionPlan>>) -> Result<Arc<dyn ExecutionPlan>>;
```
Which can be invoked as `ExecutionPlan::with_new_children_if_necessary(s,
children)`. Whilst perhaps not as ergonomic as
`s.with_new_children_if_necessary(children)`, its effectively a constructor
so... :shrug:
> There is other pitfall to use Trait Objects, for example:
I think this is a tad disingenuous, this is a pitfall of using pointer
equality on fat pointers, of which trait objects are just one type. If you wish
to perform pointer equality, as opposed to actually implementing PartialEq, you
should thin the pointers first. I don't think it is fair to say this is an
issue with trait objects, but rather one of the many pitfalls of using raw
pointers.
> That means the original type which implements the ExecutionPlan Trait can
not hold any non-static references
This limitation would still exist in the enumeration approach, unless a
lifetime parameter were introduced into the enum type definition, which I think
would be a very tough sell.
> And we have to implement the as_any() method for all the Trait
implementations, so that in the places where the trait
is consumed, it can be downcast to the original type.
In most cases I suspect you are downcasting to a single concrete type in
which case
```
if let Some(foo) = plan.as_any().downcast_ref::<Sum>() {
}
```
vs
```
if let Sum(foo) = &plan {
}
```
Is not a major difference imo... The major pain comes when matching multiple
types, in many cases I think this would be better avoided by lifting into a
member function on the trait, but lets say there is some use-case where this is
not possible. Perhaps we could introduce an enum like
```
enum ConcreteExecutionPlan<'a> {
Sum(&'a Sum),
Max(&'a Max),
...
}
```
And then add something like
```
fn downcast(&dyn ExecutionPlan) -> Option<ConcreteExecutionPlan<'_>> {}
```
I dunno, I'm not really sure I understand the use-case for enumerating the
concrete ExecutionPlan types...
--
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]
