phillipleblanc commented on code in PR #12331:
URL: https://github.com/apache/datafusion/pull/12331#discussion_r1744845781
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -450,10 +453,33 @@ impl Unparser<'_> {
Ok(())
}
LogicalPlan::SubqueryAlias(plan_alias) => {
- // Handle bottom-up to allocate relation
- let (plan, columns) =
subquery_alias_inner_query_and_columns(plan_alias);
+ let (plan, mut columns) =
+ subquery_alias_inner_query_and_columns(plan_alias);
+
+ if !columns.is_empty()
+ && !self.dialect.supports_column_alias_in_table_alias()
+ {
+ // if columns are returned than plan corresponds to a
projection
Review Comment:
```suggestion
// if columns are returned then plan corresponds to a
projection
```
##########
datafusion/sql/src/unparser/rewrite.rs:
##########
@@ -257,6 +257,37 @@ pub(super) fn subquery_alias_inner_query_and_columns(
(outer_projections.input.as_ref(), columns)
}
+/// Injects column aliases into the projection of a logical plan by wrapping
`Expr::Column` expressions
+/// with `Expr::Alias` using the provided list of aliases. Non-column
expressions are left unchanged.
+///
+/// Example:
+/// - `SELECT col1, col2 FROM table` with aliases `["alias_1",
"some_alias_2"]` will be transformed to
+/// - `SELECT col1 AS alias_1, col2 AS some_alias_2 FROM table`
+pub(super) fn inject_column_aliases(
+ projection: &datafusion_expr::Projection,
+ aliases: &Vec<Ident>,
+) -> LogicalPlan {
+ let mut updated_projection = projection.clone();
+
+ let new_exprs = projection
+ .expr
+ .iter()
+ .zip(aliases)
+ .map(|(expr, col_alias)| match expr {
+ Expr::Column(col) => Expr::Alias(Alias {
+ expr: Box::new(expr.clone()),
+ relation: col.relation.clone(),
+ name: col_alias.to_string(),
+ }),
+ _ => expr.clone(),
+ })
+ .collect::<Vec<_>>();
+
+ updated_projection.expr.clone_from(&new_exprs);
Review Comment:
We can avoid a few unnecessary clones by accepting an `impl
IntoIterator<Ident>` for `aliases` (since the caller doesn't actually need them
anymore). Then the calling code becomes:
```rust
let rewritten_plan = inject_column_aliases(inner_p, columns);
columns = vec![];
```
And this can be:
```rust
pub(super) fn inject_column_aliases(
projection: &datafusion_expr::Projection,
aliases: impl IntoIterator<Item = Ident>,
) -> LogicalPlan {
let mut updated_projection = projection.clone();
let new_exprs = updated_projection
.expr
.into_iter()
.zip(aliases)
.map(|(expr, col_alias)| match expr {
Expr::Column(col) => {
let relation = col.relation.clone();
Expr::Alias(Alias {
expr: Box::new(Expr::Column(col)),
relation,
name: col_alias.value,
})
}
_ => expr,
})
.collect::<Vec<_>>();
updated_projection.expr = new_exprs;
LogicalPlan::Projection(updated_projection)
}
```
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -450,10 +453,33 @@ impl Unparser<'_> {
Ok(())
}
LogicalPlan::SubqueryAlias(plan_alias) => {
- // Handle bottom-up to allocate relation
- let (plan, columns) =
subquery_alias_inner_query_and_columns(plan_alias);
+ let (plan, mut columns) =
+ subquery_alias_inner_query_and_columns(plan_alias);
+
+ if !columns.is_empty()
+ && !self.dialect.supports_column_alias_in_table_alias()
+ {
+ // if columns are returned than plan corresponds to a
projection
+ let LogicalPlan::Projection(inner_p) = plan else {
+ return plan_err!(
+ "Inner projection for subquery alias is expected"
+ );
+ };
+
+ // Instead of specifying column aliases as part of the
outer table inject them directly into the inner projection
Review Comment:
```suggestion
// Instead of specifying column aliases as part of the
outer table, inject them directly into the inner projection
```
##########
datafusion/sql/src/unparser/rewrite.rs:
##########
@@ -257,6 +257,37 @@ pub(super) fn subquery_alias_inner_query_and_columns(
(outer_projections.input.as_ref(), columns)
}
+/// Injects column aliases into the projection of a logical plan by wrapping
`Expr::Column` expressions
+/// with `Expr::Alias` using the provided list of aliases. Non-column
expressions are left unchanged.
+///
+/// Example:
+/// - `SELECT col1, col2 FROM table` with aliases `["alias_1",
"some_alias_2"]` will be transformed to
+/// - `SELECT col1 AS alias_1, col2 AS some_alias_2 FROM table`
+pub(super) fn inject_column_aliases(
+ projection: &datafusion_expr::Projection,
+ aliases: &Vec<Ident>,
+) -> LogicalPlan {
+ let mut updated_projection = projection.clone();
+
+ let new_exprs = projection
+ .expr
+ .iter()
+ .zip(aliases)
+ .map(|(expr, col_alias)| match expr {
+ Expr::Column(col) => Expr::Alias(Alias {
+ expr: Box::new(expr.clone()),
+ relation: col.relation.clone(),
+ name: col_alias.to_string(),
+ }),
+ _ => expr.clone(),
+ })
+ .collect::<Vec<_>>();
+
+ updated_projection.expr.clone_from(&new_exprs);
Review Comment:
Cloning `TableReference` is cheap, so `col.relation.clone()` is lightweight.
The only clone we need to do now is `projection.clone()` which I don't think
there is a way around.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
