alamb commented on a change in pull request #1067:
URL: https://github.com/apache/arrow-datafusion/pull/1067#discussion_r721250543
##########
File path: datafusion/src/sql/planner.rs
##########
@@ -522,21 +521,43 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
name
))),
}?,
- columns_alias,
+ alias,
)
}
TableFactor::Derived {
subquery, alias, ..
- } => (
- self.query_to_plan_with_alias(
+ } => {
+ // if alias is None, return Err
+ if alias.is_none() {
+ return Err(DataFusionError::Plan(
+ "subquery in FROM must have an alias".parse().unwrap(),
+ ));
+ }
+ let logical_plan = self.query_to_plan_with_alias(
subquery,
alias.as_ref().map(|a| a.name.value.to_string()),
ctes,
- )?,
- alias.clone().map(|x| x.columns),
- ),
+ )?;
+ let schema = logical_plan.schema();
Review comment:
I don't understand the need for this special case handling of
`TableFactor::Derived` -- it seems like the rewriting of the alias is done at
the end of this function -- aka lines 583 where you have changed the code that
adds a projection (to implement the aliases) to also include the table name
alias, if available,
In other words, I would have expected that
https://github.com/apache/arrow-datafusion/pull/1067/files#diff-36ac5b22d82d96b989febf9d57d8c2b5fab46122ff1268fb2c400b02542f0f63R583-R594
alone would be good enough to fix this issue
##########
File path: datafusion/src/sql/planner.rs
##########
@@ -522,21 +521,43 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
name
))),
}?,
- columns_alias,
+ alias,
)
}
TableFactor::Derived {
subquery, alias, ..
- } => (
- self.query_to_plan_with_alias(
+ } => {
+ // if alias is None, return Err
+ if alias.is_none() {
+ return Err(DataFusionError::Plan(
+ "subquery in FROM must have an alias".parse().unwrap(),
Review comment:
```suggestion
"subquery in FROM must have an alias".to_string(),
```
I think you can just create a `String` here
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -298,6 +298,19 @@ impl LogicalPlan {
| LogicalPlan::Filter { input, .. } => input.all_schemas(),
}
}
+ /// schema to projection logical plan
Review comment:
This code seems like it tries to specify that the output schema's
relation name (table alias) that is different than than the input schema's
relation.
I think what is desired is a new LogicalPlan::Projection that changed the
schema names rather than trying to rewrite them.
For example, to change the table alias from `a` to `b`
If the input was like
```
LogicalPlan::Projection(schema = {a.c1, a.c2}, expr: [a.c1, a.c2])
```
As @houqp says we should add a single new LogicalPlan node like:
```
LogicalPlan::Projection(schema = {b.c1, b.c2}, expr: [a.c1, a.c2])
LogicalPlan::Projection(schema = {a.c1, a.c2}, expr: [a.c1, a.c2])
```
(in other words, don't try and rewrite the existing
`LogicalPlan::Projection`, but put a new one on the top that changes the schema)
If you use @houqp 's suggestion to add an optional alias to
`LogicalPlan::Projection` then the top LogicalPlan can be created
##########
File path: datafusion/src/sql/planner.rs
##########
@@ -522,21 +521,43 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
name
))),
}?,
- columns_alias,
+ alias,
)
}
TableFactor::Derived {
subquery, alias, ..
- } => (
- self.query_to_plan_with_alias(
+ } => {
+ // if alias is None, return Err
+ if alias.is_none() {
Review comment:
FWIW this is consistent with Postgres:
```
alamb=# select * from public.simple as a join (select * from public.simple)
on a.c3;
ERROR: subquery in FROM must have an alias
LINE 1: select * from public.simple as a join (select * from public....
^
HINT: For example, FROM (SELECT ...) [AS] foo.
```
👍
--
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]
