jayshrivastava commented on code in PR #24018: URL: https://github.com/apache/datafusion/pull/24018#discussion_r3742124344
########## datafusion/physical-plan/src/memory.rs: ########## Review Comment: Good point. The behavior in this PR is inconsistent, so I just pushed a commit with a comment to define the behavior: I think we should ignore expressions in the `properties()` for several reasons 1. It's a lot of code to have every `ExecutionPlan` has to visit the sort expr and the partitioning expr in the properties 2. Many `ExecutionPlan` nodes just return `self.children().properties()`, so it's redundant 3. I doubt users will want `apply_expressions` to yield expressions in properties. I think users would only care about expressions that are evaluated and important during execution. 4. You can already call `properties()` if you want the properties Here's the updated doc comment: ``` /// Apply a closure `f` to each root expression that this node owns and uses /// during execution, either by evaluating it or updating it dynamically. /// /// An expression must not be visited solely because it describes an input or /// output property, such as cached ordering, partitioning, or equivalence /// metadata. However, these may be traversed indirectly. For example, /// `RepartitionExec` visits the partitioning expressions it evaluates and /// `SortExec` visits the sort expressions it evaluates to order rows. /// /// This method is shallow: it must not visit expression children or expressions /// owned by child execution plans. /// /// Similarly to other [`TreeNode`] APIs, the closure can return ``` Let me know if this makes sense. -- 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]
