-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add SQL planner, physical planner, and TableProvider hook for MERGE INTO #22988
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ use crate::{ | |
| DistinctOn, DmlStatement, Execute, Explain, Expr, Extension, Filter, Join, Limit, | ||
| LogicalPlan, Partitioning, Prepare, Projection, RecursiveQuery, Repartition, Sort, | ||
| Statement, Subquery, SubqueryAlias, TableScan, Union, Unnest, UserDefinedLogicalNode, | ||
| Values, Window, builder::unnest_with_options, dml::CopyTo, | ||
| Values, Window, WriteOp, builder::unnest_with_options, dml::CopyTo, | ||
| }; | ||
| use datafusion_common::tree_node::TreeNodeRefContainer; | ||
|
|
||
|
|
@@ -480,6 +480,10 @@ impl LogicalPlan { | |
| } | ||
| _ => Ok(TreeNodeRecursion::Continue), | ||
| }, | ||
| LogicalPlan::Dml(DmlStatement { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should avoid exposing The analyzer and type coercion rewrite schema is built from Could we either keep these expressions out of generic traversal for the initial implementation, or store enough target schema and qualifier information on the MERGE operation so analyzer/type coercion can rewrite them against the combined target plus source schema?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went with storing target schema info on the MERGE operation. Added |
||
| op: WriteOp::MergeInto(merge_op), | ||
| .. | ||
| }) => merge_op.exprs().apply_ref_elements(f), | ||
| // plans without expressions | ||
| LogicalPlan::EmptyRelation(_) | ||
| | LogicalPlan::RecursiveQuery(_) | ||
|
|
@@ -719,6 +723,27 @@ impl LogicalPlan { | |
| ) | ||
| })? | ||
| } | ||
| LogicalPlan::Dml(DmlStatement { | ||
| table_name, | ||
| target, | ||
| op: WriteOp::MergeInto(merge_op), | ||
| input, | ||
| output_schema, | ||
| }) => { | ||
| let owned_exprs: Vec<Expr> = | ||
| merge_op.exprs().into_iter().cloned().collect(); | ||
| owned_exprs.map_elements(f)?.transform_data(|new_exprs| { | ||
| Ok(Transformed::no(LogicalPlan::Dml(DmlStatement { | ||
| table_name, | ||
| target, | ||
| op: WriteOp::MergeInto(Box::new( | ||
| merge_op.with_new_exprs(new_exprs)?, | ||
| )), | ||
| input, | ||
| output_schema, | ||
| }))) | ||
| })? | ||
| } | ||
| // plans without expressions | ||
| LogicalPlan::EmptyRelation(_) | ||
| | LogicalPlan::RecursiveQuery(_) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid adding
target_schemaas a new public field onMergeIntoOp?Because
MergeIntoOpis a public struct that downstream users can construct with struct literals, adding a required field is a SemVer-breaking change.I do not think that break is necessary for this PR.
target_schemalooks like analyzer and planning context rather than essential public MERGE operation data. Exposing it here makes downstream callers provide internal state they should not need to know about.Can we keep the additional planning state outside the publicly constructible struct, or introduce a SemVer-compatible constructor or internal representation before requiring new state from callers?
One possible approach is to keep
MergeIntoOp { on, clauses }, derive the target schema in the analyzer, type coercion, or function rewrite passes from the existingDmlStatement { table_name, target, .. }, and canonicalize alias-qualified target references during SQL planning.