You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "phillipleblanc (via GitHub)" <gi...@apache.org> on 2024/05/29 07:03:30 UTC
[PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
phillipleblanc opened a new pull request, #10707:
URL: https://github.com/apache/datafusion/pull/10707
## Which issue does this PR close?
Closes #10706
## Rationale for this change
The schema that is the result of a UNION ALL should not have any table qualifiers, as the table information has effectively been erased and is no longer a valid reference.
Consider the following SQL:
```sql
SELECT table1.foo FROM table1
UNION ALL
SELECT table2.foo FROM table2
```
The logical schema from this UNION ALL should be just `foo`, but it is currently taking the first input's schema directly, resulting in a schema of `table1.foo`.
This came up as an issue when trying to validate the unparser works correctly for UNION ALL statements, which I added in https://github.com/apache/datafusion/pull/10603
Slightly modifying the above example, if I add an ORDER BY clause to the input SQL:
```sql
SELECT table1.foo FROM table1
UNION ALL
SELECT table2.foo FROM table2
ORDER BY foo
```
Then the resulting unparsed SQL will be:
```sql
SELECT table1.foo FROM table1
UNION ALL
SELECT table2.foo FROM table2
ORDER BY table1.foo
```
Because the unparser takes the schema directly from the Union node when writing out the column expressions.
## What changes are included in this PR?
Fixes the `LogicalPlanBuilder::union` to not take the first input's schema directly, but sets it to `None`.
## Are these changes tested?
Yes, I added a test both in the LogicalPlanBuilder and in the Unparser `sql_integration` test.
## Are there any user-facing changes?
No
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #10707:
URL: https://github.com/apache/datafusion/pull/10707#issuecomment-2185117290
Closing in favor of #11082
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #10707:
URL: https://github.com/apache/datafusion/pull/10707#issuecomment-2138431808
Marking as draft while I dig into the `DuplicateUnqualifiedField` issue
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #10707:
URL: https://github.com/apache/datafusion/pull/10707#discussion_r1650021623
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1373,33 +1373,32 @@ pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalP
}
// create union schema
- let union_qualified_fields =
+ let union_fields =
zip(left_plan.schema().iter(), right_plan.schema().iter())
.map(
- |((left_qualifier, left_field), (_right_qualifier, right_field))| {
+ |((_, left_field), (_, right_field))| {
Review Comment:
Asked on Discord why we need to enforce the `DuplicateUnqualifiedField` check - Postgres itself does not error if two columns have the same name.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #10707:
URL: https://github.com/apache/datafusion/pull/10707#discussion_r1650118809
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1373,33 +1373,32 @@ pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalP
}
// create union schema
- let union_qualified_fields =
+ let union_fields =
zip(left_plan.schema().iter(), right_plan.schema().iter())
.map(
- |((left_qualifier, left_field), (_right_qualifier, right_field))| {
+ |((_, left_field), (_, right_field))| {
Review Comment:
I've raised #11082 as an alternative way to do this that only affects the Unparser.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #10707:
URL: https://github.com/apache/datafusion/pull/10707#discussion_r1618338309
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1373,33 +1373,32 @@ pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalP
}
// create union schema
- let union_qualified_fields =
+ let union_fields =
zip(left_plan.schema().iter(), right_plan.schema().iter())
.map(
- |((left_qualifier, left_field), (_right_qualifier, right_field))| {
+ |((_, left_field), (_, right_field))| {
Review Comment:
There is a test that failed because of this change, and looking into the history the reason this was done was to avoid the following error:
https://github.com/apache/datafusion/issues/5410
`SchemaError(DuplicateUnqualifiedField { name: "a" })`
So it seems that this is intentional 🤔 If this only affects the Unparser, I could try stripping the qualifier only from the `Sort` node if the input node is a `Union`, but that seems a bit hacky to me.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #10707:
URL: https://github.com/apache/datafusion/pull/10707#discussion_r1650021623
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1373,33 +1373,32 @@ pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalP
}
// create union schema
- let union_qualified_fields =
+ let union_fields =
zip(left_plan.schema().iter(), right_plan.schema().iter())
.map(
- |((left_qualifier, left_field), (_right_qualifier, right_field))| {
+ |((_, left_field), (_, right_field))| {
Review Comment:
Asked on [Discord] why we need to enforce the `DuplicateUnqualifiedField` check - Postgres itself does not error if two columns have the same name.
[Discord]: https://discord.com/channels/885562378132000778/1166447479609376850/1254355115973021788
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #10707:
URL: https://github.com/apache/datafusion/pull/10707#discussion_r1618341153
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1373,33 +1373,32 @@ pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalP
}
// create union schema
- let union_qualified_fields =
+ let union_fields =
zip(left_plan.schema().iter(), right_plan.schema().iter())
.map(
- |((left_qualifier, left_field), (_right_qualifier, right_field))| {
+ |((_, left_field), (_, right_field))| {
Review Comment:
Further discussion that seems to indicate that removing the qualifier is the correct behavior: #2943
I'll dig into if there is a way around the `DuplicateUnqualifiedField`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Strip table qualifiers from schema in `UNION ALL` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc closed pull request #10707: Strip table qualifiers from schema in `UNION ALL`
URL: https://github.com/apache/datafusion/pull/10707
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org