Add LogicalPlan::recompute_schema for handling rewrite passes
#10410
Conversation
| /// # Notes | ||
| /// | ||
| /// * Does not recursively recompute schema for input (child) plans. | ||
| pub fn recompute_schema(self) -> Result<Self> { |
There was a problem hiding this comment.
All this logic is the same as new_with_expr, even some questionable code like for Filter and Union
There was a problem hiding this comment.
I have a question which may be stupid, what's the difference between this and getting the inputs and exprs of the LogicalPlan and pass them as params to new_with_expr? 🤔
There was a problem hiding this comment.
The key difference is that to use new_with_expr it requires providing a copy (Vec<Expr> and Vec<LogicalPlan>) where as this function can be used after modifying the Exprs (or children) via methods such as map_children and map_expressions
Avoiding those copies is a key part of improve planning performance in PRs like #10356 (the change is basically to stop copying)
There was a problem hiding this comment.
I pushed bd7b62f to try and clarify
| @@ -3138,4 +3350,86 @@ digraph { | |||
| let actual = format!("{}", plan.display_indent()); | |||
| assert_eq!(expected.to_string(), actual) | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
Here are some basic tests that show what it does and how to use it.
|
Included as part of #10410, so closing this PR |
Which issue does this PR close?
Part of #9637
Rationale for this change
LogicalPlan::recompute_schemais needed for several subtasks of #9637 (for example #10356 and #10405)I felt making its own PR with tests might make those PRs easier to review
What changes are included in this PR?
LogicalPlan::recompute_schemaLogicalPlan::recompute_schemaAre these changes tested?
Yes, new tests (though full coverage will come in #10356 and #10405)
Are there any user-facing changes?