Fix invalid schema for unions in ViewTables

[Friede80]

Which issue does this PR close?

• Closes Invalid schema for unions in ViewTable #15134

Rationale for this change

At the core, if a Union goes through the TypeCoercion analyzer twice, the schema can drop the qualifier information for fields that were cast in the first pass of TypeCoercion. To preserve this information, this PR changes coerce_union_schema to start from the schema of the Union instead of the schema of the first child.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, this changes the signature of coerce_union_schema so that the schema carried by the Union is available.

Before:

pub fn coerce_union_schema(inputs: &[Arc<LogicalPlan>]) -> Result<DFSchema>

After:

pub fn coerce_union_schema(union_plan: &Union) -> Result<DFSchema>

[Friede80]

I'm not sure if there are still valid uses of coerce_union_schema given only the set of logical plans, but if we can't change the api of a public function, it would be easy enough to accomplish this with a wrapper. i.e.

pub fn coerce_union_schema(inputs: &[Arc<LogicalPlan>]) -> Result<DFSchema> {
    coerce_union_schema_with_schema(inputs, inputs[0].schema())
}
fn coerce_union_schema_with_schema(inputs: &[Arc<LogicalPlan>], base_schema: &DFSchemaRef) -> Result<DFSchema> {
...
}

[Friede80]

There seems to have been quite a few cracks at this particular situation:

• Join two tables with the same schema, then union throws SchemaError(DuplicateUnqualifiedField ) #5410
• Strip table qualifiers from schema in UNION ALL for unparser #11082
• feat: Implement UNION ALL BY NAME #14538
• Strip table qualifiers from schema in UNION ALL #10707

The trouble seems to stem from the decision to maintain TableReference qualifiers in unions for disambiguation. This makes it very difficult to know when these qualifiers should or shouldn't be passed through at different points in the optimizer. Unless there are well specified semantics around this that I'm missing, I think it would be better go back to the assumption that qualifiers never persist through a union. And we can continue to support field disambiguation by generating unique column names as is done in DuckDB (deduplicating-identifiers)

[alamb]

There seems to have been quite a few cracks at this particular situation:

• Join two tables with the same schema, then union throws SchemaError(DuplicateUnqualifiedField ) #5410
• Strip table qualifiers from schema in UNION ALL for unparser #11082
• feat: Implement UNION ALL BY NAME #14538
• Strip table qualifiers from schema in UNION ALL #10707

The trouble seems to stem from the decision to maintain TableReference qualifiers in unions for disambiguation. This makes it very difficult to know when these qualifiers should or shouldn't be passed through at different points in the optimizer. Unless there are well specified semantics around this that I'm missing, I think it would be better go back to the assumption that qualifiers never persist through a union. And we can continue to support field disambiguation by generating unique column names as is done in DuckDB (deduplicating-identifiers)

Maybe @jonahgao can offer an opinion on this suggestion

[jonahgao]

Why do we need this? It seems to make the following invalid query executable.

DataFusion CLI v46.0.0
> create table t1(a int);
0 row(s) fetched.
Elapsed 0.007 seconds.

> create table t2(a int);
0 row(s) fetched.
Elapsed 0.007 seconds.

> select t1.a from t1 union all select t2.a from t2 order by not_exist_table.a;
+---+
| a |
+---+
+---+
0 row(s) fetched.
Elapsed 0.008 seconds.

[Friede80]

I agree it feels strange and I wouldn't mind getting rid of this, but I put it in for backwards compatibility, because currently if you ran:

select t1.a from t1 union all select t2.a from t2 order by t1.a

it would work. But after this PR, it would fail without this pass

[jonahgao]

Since we stripped qualifiers, it should be reasonable that order by t1.a fails. I tested this behavior in PostgreSQL, and it works that way.

psql=> select a from t1 union all select a from t2 order by t1.a;
ERROR:  missing FROM-clause entry for table "t1"
LINE 1: select a from t1 union all select a from t2 order by t1.a;
                                                             ^
psql=> select a from t1 union all select a from t2 order by a;
 a
---
(0 rows)

Moreover, it seems like a bug to me if order by not_exist_table.a executes successfully.

[jonahgao]

Make sense to me👍. But it doesn't work for this case:

> create table t1(a int);
0 row(s) fetched.
Elapsed 0.005 seconds.

> create table t2(a int);
0 row(s) fetched.
Elapsed 0.007 seconds.

> select * from t1,t2 union all select * from t1,t2;
type_coercion
caused by
Schema error: Schema contains duplicate unqualified field name

The above query works on the main branch.

[jonahgao]

It seems like TypeCoercion undoes the renaming efforts here.
Maybe TypeCoercion should only modify the data types of union inputs, rather than field names 🤔

[Friede80]

oh right, because the union schema is discarded during coercion. My initial attempt to fix this issue propagated the union schema through, but I removed it when I decided not to maintain qualifiers, but I think adding that back would resolve this.

[Friede80]

Addressed in 612c217