Update API for extension planning to include logical plan#643
Update API for extension planning to include logical plan#643alamb merged 2 commits intoapache:masterfrom
Conversation
| /// This errors when the planner knows how to plan the concrete implementation of `node` | ||
| /// but errors while doing so, and `None` when the planner does not know how to plan the `node` | ||
| /// and wants to delegate the planning to another [`ExtensionPlanner`]. | ||
| /// |
There was a problem hiding this comment.
The challenge prior to this PR is if the extension node has any Exprs to convert them into PhysicalExprs requires access to the LogicalPlan
Furthermore, there was no way to call back into create_physical_expr from the ExtensionPlanner so I added that to the trait as well
|
Thank you for the review @houqp. Any thoughts @andygrove / @Dandandan / @jorgecarleitao ? Otherwise I would like to merge this in the next day or so. |
jorgecarleitao
left a comment
jorgecarleitao
left a comment
There was a problem hiding this comment.
Yeap, clear for me. Great simplification and also some nice doc strings 👍
| /// `input_schema`: the physical schema for evaluating `e` | ||
| fn create_physical_expr( | ||
| &self, | ||
| e: &Expr, |
There was a problem hiding this comment.
Nit: why call it e? I think expr as variable name is more consistent with the whole codebase?
There was a problem hiding this comment.
I agree expr is more consistent -- I think I was following the model of DefaultPhysicalPlanner::create_physical_expr. I will change
| input_schema: &Schema, | ||
| ctx_state: &ExecutionContextState, | ||
| ) -> Result<Arc<dyn PhysicalExpr>> { | ||
| self.create_physical_expr(e, input_dfschema, input_schema, ctx_state) |
There was a problem hiding this comment.
From here it looks like infinite recursion, but I might be wrong?
There was a problem hiding this comment.
It somehow knows to call DefaultPhysicalPlanner::create_physical_expr -- I will make this clear
alamb
force-pushed
the
alamb/extension_planning
branch
from
3b3112d to
d61280a
Compare
4 participants
Which issue does this PR close?
Closes #642
Rationale for this change
With the introduction of qualified identifiers in #55 I am having trouble creating plans with user defined nodes because the extension traits don't have sufficient information about the logical plan to create the correct
PhysicalExprs.#642 describes in more detail what is going on
What changes are included in this PR?
PhysicalPlanner::create_physical_exprto the planer traitExtensionPlanner::plan_extensionto include a reference to the planner as well as the logical inputsAre there any user-facing changes?
Yes: Extension API and PhysicalPlanner interfaces have changed