alamb commented on issue #21080: URL: https://github.com/apache/datafusion/issues/21080#issuecomment-4507775224
> Reading the PR [#20337](https://github.com/apache/datafusion/pull/20337) I see its intentionally without default implementation, so my question is what would be appropriate implementation for it? When it is safe to implement it as `Ok(TreeNodeRecursion::Continue)` and ignore call ? > > I believe adding default implementation would make upgrade process smoother. > > cc [@LiaCastaneda](https://github.com/LiaCastaneda) & [@alamb](https://github.com/alamb) ? Thanks @milenkovicm -- I was worried that a default implementation would lead to subtle / hard to find bugs. i think downstream systems should implement one of hte patterns listed in https://datafusion.apache.org/library-user-guide/upgrading/54.0.0.html#executionplan-apply-expressions-is-now-a-required-method We could perhaps make the migration guide clearer (that the idea is to visit all expressions that are contained in the ExecutionPlan) 🤔 ``` Node with no expressions: fn apply_expressions( &self, _f: &mut dyn FnMut(&dyn PhysicalExpr) -> Result<TreeNodeRecursion>, ) -> Result<TreeNodeRecursion> { Ok(TreeNodeRecursion::Continue) } Node with a single expression: fn apply_expressions( &self, f: &mut dyn FnMut(&dyn PhysicalExpr) -> Result<TreeNodeRecursion>, ) -> Result<TreeNodeRecursion> { f(self.predicate.as_ref()) } Node with multiple expressions: fn apply_expressions( &self, f: &mut dyn FnMut(&dyn PhysicalExpr) -> Result<TreeNodeRecursion>, ) -> Result<TreeNodeRecursion> { let mut tnr = TreeNodeRecursion::Continue; for expr in &self.expressions { tnr = tnr.visit_sibling(|| f(expr.as_ref()))?; } Ok(tnr) } ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
