-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support unparsing plans after applying optimize_projections
rule
#13267
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 |
---|---|---|
|
@@ -278,9 +278,11 @@ impl Unparser<'_> { | |
) -> Result<()> { | ||
match plan { | ||
LogicalPlan::TableScan(scan) => { | ||
if let Some(unparsed_table_scan) = | ||
Self::unparse_table_scan_pushdown(plan, None)? | ||
{ | ||
if let Some(unparsed_table_scan) = Self::unparse_table_scan_pushdown( | ||
plan, | ||
None, | ||
select.already_projected(), | ||
)? { | ||
return self.select_to_sql_recursively( | ||
&unparsed_table_scan, | ||
query, | ||
|
@@ -567,6 +569,7 @@ impl Unparser<'_> { | |
let unparsed_table_scan = Self::unparse_table_scan_pushdown( | ||
plan, | ||
Some(plan_alias.alias.clone()), | ||
select.already_projected(), | ||
)?; | ||
// if the child plan is a TableScan with pushdown operations, we don't need to | ||
// create an additional subquery for it | ||
|
@@ -696,6 +699,7 @@ impl Unparser<'_> { | |
fn unparse_table_scan_pushdown( | ||
plan: &LogicalPlan, | ||
alias: Option<TableReference>, | ||
already_projected: bool, | ||
) -> Result<Option<LogicalPlan>> { | ||
match plan { | ||
LogicalPlan::TableScan(table_scan) => { | ||
|
@@ -725,24 +729,29 @@ impl Unparser<'_> { | |
} | ||
} | ||
|
||
if let Some(project_vec) = &table_scan.projection { | ||
let project_columns = project_vec | ||
.iter() | ||
.cloned() | ||
.map(|i| { | ||
let schema = table_scan.source.schema(); | ||
let field = schema.field(i); | ||
if alias.is_some() { | ||
Column::new(alias.clone(), field.name().clone()) | ||
} else { | ||
Column::new( | ||
Some(table_scan.table_name.clone()), | ||
field.name().clone(), | ||
) | ||
} | ||
}) | ||
.collect::<Vec<_>>(); | ||
builder = builder.project(project_columns)?; | ||
// Avoid creating a duplicate Projection node, which would result in an additional subquery if a projection already exists. | ||
// For example, if the `optimize_projection` rule is applied, there will be a Projection node, and duplicate projection | ||
// information included in the TableScan node. | ||
if !already_projected { | ||
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. This prevents from generating queries like |
||
if let Some(project_vec) = &table_scan.projection { | ||
let project_columns = project_vec | ||
.iter() | ||
.cloned() | ||
.map(|i| { | ||
let schema = table_scan.source.schema(); | ||
let field = schema.field(i); | ||
if alias.is_some() { | ||
Column::new(alias.clone(), field.name().clone()) | ||
} else { | ||
Column::new( | ||
Some(table_scan.table_name.clone()), | ||
field.name().clone(), | ||
) | ||
} | ||
}) | ||
.collect::<Vec<_>>(); | ||
builder = builder.project(project_columns)?; | ||
} | ||
} | ||
|
||
let filter_expr: Result<Option<Expr>> = table_scan | ||
|
@@ -787,14 +796,17 @@ impl Unparser<'_> { | |
Self::unparse_table_scan_pushdown( | ||
&subquery_alias.input, | ||
Some(subquery_alias.alias.clone()), | ||
already_projected, | ||
) | ||
} | ||
// SubqueryAlias could be rewritten to a plan with a projection as the top node by [rewrite::subquery_alias_inner_query_and_columns]. | ||
// The inner table scan could be a scan with pushdown operations. | ||
LogicalPlan::Projection(projection) => { | ||
if let Some(plan) = | ||
Self::unparse_table_scan_pushdown(&projection.input, alias.clone())? | ||
{ | ||
if let Some(plan) = Self::unparse_table_scan_pushdown( | ||
&projection.input, | ||
alias.clone(), | ||
already_projected, | ||
)? { | ||
let exprs = if alias.is_some() { | ||
let mut alias_rewriter = | ||
alias.as_ref().map(|alias_name| TableAliasRewriter { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -882,6 +882,7 @@ fn test_table_scan_pushdown() -> Result<()> { | |
let query_from_table_scan_with_projection = LogicalPlanBuilder::from( | ||
table_scan(Some("t1"), &schema, Some(vec![0, 1]))?.build()?, | ||
) | ||
.project(vec![col("id"), col("age")])? | ||
.project(vec![wildcard()])? | ||
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. Can this actually be a real plan with a wildcard projection and a TableScan that includes only two columns? I would expect them to match. If this is a real use case I will improve logic above (check for parent projection is a wildcard or does not match). Running all TPC-H and TPC-DS queries I've not found query where it was the case. |
||
.build()?; | ||
let query_from_table_scan_with_projection = | ||
|
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.
Might it be better to make this flag more generic, for example, just
preserve_existing_projections
orprefer_existing_plan_nodes
, so it can be reused in the future in similar cases