mingmwang commented on issue #2175: URL: https://github.com/apache/arrow-datafusion/issues/2175#issuecomment-1094883368
Besides the code style, the reason that I prefer to use Enum than Trait/Trait Objects is because Trait is simple but Trait Object is a quite complex concept in Rust and not all the Traits can be used as Trait Objects: the return type of the Trait can not be the 'Self' and there should be no generic types parameters. For example in this PR [https://github.com/apache/arrow-datafusion/pull/2168](https://github.com/apache/arrow-datafusion/pull/2168), the new method `with_new_children_if_necessary` can not be implemented as a Trait method ```` /// Can not compile fn with_new_children_if_necessary( self: Arc<Self>, children: Vec<Arc<dyn ExecutionPlan>>, ) -> Result<Arc<dyn ExecutionPlan>> { if children.len() != self.children().len() { Err(DataFusionError::Internal( "Wrong number of children".to_string(), )) } else if children.is_empty() || children .iter() .zip(self.children().iter()) .any(|(c1, c2)| !Arc::ptr_eq(c1, c2)) { self.with_new_children(children) } else { Ok(self) } } ```` ```` /// Can not compile fn with_new_children_if_necessary( &self, children: Vec<Arc<dyn ExecutionPlan>>, ) -> Result<Arc<dyn ExecutionPlan>> { if children.len() != self.children().len() { Err(DataFusionError::Internal( "Wrong number of children".to_string(), )) } else if children.is_empty() || children .iter() .zip(self.children().iter()) .any(|(c1, c2)| !Arc::ptr_eq(c1, c2)) { self.with_new_children(children) } else { Ok(Arc::new(self.clone())) } } ```` ```` /// Can not compile fn with_new_children_if_necessary( self: Arc<Self>, children: Vec<Arc<dyn ExecutionPlan>>, ) -> Result<Arc<Self>> { if children.len() != self.children().len() { Err(DataFusionError::Internal( "Wrong number of children".to_string(), )) } else if children.is_empty() || children .iter() .zip(self.children().iter()) .any(|(c1, c2)| !Arc::ptr_eq(c1, c2)) { self.with_new_children(children) } else { Ok(self) } } ```` 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. And the Any Trait is qualified with 'static lifecycle. That means the original type which implements the ExecutionPlan Trait can not hold any non-static references, the Struct that implements the Trait must be owned type. ```` fn as_any(&self) -> &dyn Any; ```` There is other pitfall to use Trait Objects, for example: [https://stackoverflow.com/questions/67109860/how-to-compare-trait-objects-within-an-arc](https://stackoverflow.com/questions/67109860/how-to-compare-trait-objects-within-an-arc) I searched the internet and find this blog, I think the points are quite valid. https://bennetthardwick.com/dont-use-boxed-trait-objects-for-struct-internals/ From extension point of view, using Enum is not that straightforward compared with Trait, but I think we can follow the same way as the LogicalPlan, just like the `UserDefinedLogicalNode` we already have for the LogicalPlan. -- 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]
