Add a utility function to get all of the PartitionedFile for an ExecutionPlan

[yahoNanJing]

Which issue does this PR close?

Closes #5566.

Rationale for this change

Currently the TreeNodeRewriter is as a visitor to transform a node to another. However, sometimes we don't need to do the transformation and what we want is only to collect some info from the node. To achieve this, it's better to introduce another visitor for collecting info and keep the node unchanged.

What changes are included in this PR?

• Introduced a common tree node visitor trait
• Implemented the visitable trait for ExecutionPlan
• Introduced a utility function to get all of the PartitionedFile for an ExecutionPlan based on the visitor mentioned above.

Are these changes tested?

Are there any user-facing changes?

[yahoNanJing]

Hi @mingmwang, @alamb, could you help review this PR?

[mingmwang]

I think you can move the TreeNodeRewriter to the new added tree_node.rs also.

[alamb]

@mustafasrepo do you have time to review this PR?

[yahoNanJing]

I think you can move the TreeNodeRewriter to the new added tree_node.rs also.

I'll add another commit to this PR for this.

[mustafasrepo]

@mustafasrepo do you have time to review this PR?

Sure. I will look at it.

[yahoNanJing]

Unfortunately, due to the Orphan rule, I fail to combine the trait TreeNodeRewriter for both of the ExecutionPan and PhysicalExpr

[mustafasrepo]

Is there any reason for implementing TreeNodeRewritable trait in another file. I guess, you could have implement this trait in the file where ExecutionPlan is implemented. I am not familiar with these pattern. I am asking to understand better.

[yahoNanJing]

If the Orphan rule is relaxed in the future, the same trait of TreeNodeRewritable can be used for the PhysicalExpr. The traits defined in the tree_node file are common abstractions. They are not just for ExecutionPlan

[mustafasrepo]

I understand why TreeNodeRewritable trait is defined in datafusion/core/src/physical_plan/tree_node/mod.rs. What I mean was that Arc<dyn ExecutionPlan> implements TreeNodeRewritable trait. I guess, we could do this implementation in datafusion/core/src/physical_plan/mod.rs. However, it is just a stylistic issue. I just wondered the reason why you did it in datafusion/core/src/physical_plan/tree_node/rewritable.rs

[mustafasrepo]

Similar to the above case I guess TreeNodeVisitable could have been implemented where ExecutionPlan is defined. Is there any reason for this specific pattern.

[mustafasrepo]

LGTM!. Thanks @yahoNanJing.

[yahoNanJing]

Thanks @mustafasrepo. I'll merge this PR.

[mingmwang]

@yahoNanJing
I think if we just to collect/search some information from a tree like structure, we do not need to define a Visitor Trait.
A visitor in this case is actually just a map function (closure) we want to apply to a tree structure.
For example, if we want to traverse a Vec/List container and collect/search some information, we do not need to define a Visitor.

[yahoNanJing]

Thanks @mingmwang for your suggestion. I'll raise another PR to replace the Visitor by closure which will be much easier to use.