Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a common trait TreeNode for ExecutionPlan, PhysicalExpr, LogicalExpr, LogicalPlan #5609

Closed
yahoNanJing opened this issue Mar 15, 2023 · 0 comments · Fixed by #5630
Labels
enhancement New feature or request

Comments

@yahoNanJing
Copy link
Contributor

yahoNanJing commented Mar 15, 2023

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Currently, there are many duplicated code related to the visitor trait and rewrite trait. And the trait methods are sometimes misused.

Describe the solution you'd like

Combine the visitor and rewrite trait to be one trait TreeNode and provide common behavior for ExecutionPlan, PhysicalExpr, LogicalExpr, LogicalPlan. And this trait contains the following methods:

  • fn get_children(&self) -> Vec<Self>; for getting its children
  • fn collect<F>(&self, op: &mut F) -> Result<()> where F: FnMut(&Self) -> Result<Recursion>, for collecting info from the tree node by a closure
  • fn visit<V: TreeNodeVisitor<N = Self>>(&self, visitor: &mut V) -> Result<Recursion> for collecting info from the tree node by a visitor. This is used when need to collect info from a specific post_visit. If only pre_visit is needed, then the collect method is preferred.
  • fn transform_down<F>(self, op: &F) -> Result<Self> where F: Fn(Self) -> Result<Option<Self>>, for rewriting the node from top to down
  • fn transform_up<F>(self, op: &F) -> Result<Self> where F: Fn(Self) -> Result<Option<Self>>, for rewriting the node from bottom to up
  • fn rewrite<R: TreeNodeRewriter<N = Self>>(self, rewriter: &mut R) -> Result<Self> for rewriting the node from top to down. This is used when need to invoke a specific pre_visit. Otherwise, either the transform_down or transform_up is preferred.
  • fn map_children<F>(self, transform: F) -> Result<Self> where F: FnMut(Self) -> Result<Self> is for replacing all of its children with the rewritted ones

Describe alternatives you've considered

Additional context

@yahoNanJing yahoNanJing added the enhancement New feature or request label Mar 15, 2023
@yahoNanJing yahoNanJing changed the title Replace the Visitor by Closure Introduce a common trait TreeNode for ExecutionPlan, PhysicalExpr, LogicalExpr, LogicalPlan Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
1 participant