Implement user defined planner for create_struct & create_named_struct
#11273
Conversation
2151a83 to
30672ce
Compare
create_structcreate_struct & create_named_struct
# Conflicts: # datafusion/sql/src/expr/mod.rs
| for planner in self.planners.iter() { | ||
| match planner.plan_struct_literal(create_struct_args, is_named_struct)? { | ||
| PlannerResult::Planned(expr) => return Ok(expr), | ||
| PlannerResult::Original(args) => create_struct_args = args, | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we have one StructPlan that returns
structornamed_structbased on the values?let mut create_struct_args = if is_named_struct { self.create_named_struct_expr(values, schema, planner_context)? } else { self.create_struct_expr(values, schema, planner_context)? };The logic here should be inside planner
@jayzhan211 Is this what u suggesting ?
If you were suggesting to move create_struct_expr & create_named_struct_expr to planner. I see an issues over there i.e these functions intnerally call self.sql_expr_to_logical_expr. I would rather have it here to keep it simple. Sorry if i understood it wrong.
There was a problem hiding this comment.
Based on my understanding we also have a named_struct function which is an alias for struct function with input expressions optionally named.
In this PR i am not moving named_struct function to udp.
My understanding was to move create_named_struct & create_struct which were solely handling struct function.
There was a problem hiding this comment.
Can we have one StructPlan that returns
structornamed_structbased on the values?let mut create_struct_args = if is_named_struct { self.create_named_struct_expr(values, schema, planner_context)? } else { self.create_struct_expr(values, schema, planner_context)? };The logic here should be inside planner
@jayzhan211 Is this what u suggesting ?
If you were suggesting to move
create_struct_expr&create_named_struct_exprto planner. I see an issues over there i.e these functions intnerally callself.sql_expr_to_logical_expr. I would rather have it here to keep it simple. Sorry if i understood it wrong.
Right, they are not possible to move into the planner. Another thing we can improve on is, forgetting about create_struct_expr and use create_named_struct_expr and named_struct. But, we can change this in next PR
There was a problem hiding this comment.
Liked mentioned earlier. Thats a great idea, i will make the change in another PR. Thanks for your help @jayzhan211
|
Thank for your time and help @jayzhan211 & @alamb |
…ruct` (apache#11273) * add UserDefinedSQLPlanner for create struct * fix linting * add create name struct user defined sql planner * simplify * refactor * refactor * remove named_struct from functions * formatting * revert 953ad31 * update docs
Which issue does this PR close?
Closes #11221
Closes #11222
Rationale for this change
Part of #11207
What changes are included in this PR?
Are these changes tested?
Using existing test cases
Are there any user-facing changes?