Using Union's input schema when recompute schema #10494
Conversation
|
This is an example. |
| ), | ||
| ))) | ||
| } | ||
| } else { | ||
| Ok(Transformed::yes(LogicalPlan::Union(Union { | ||
| inputs: new_inputs, | ||
| schema: union.schema.clone(), | ||
| schema: input_schema.clone(), |
There was a problem hiding this comment.
From the example, after propagate_empty_relation, the new inputs of Union doesn't have the Alias 'a', but only test exists.
If we want to use the new inputs's schema, it will be
DFSchema { inner: Schema { fields: [Field { name: "col_int32", data_type: Int32, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [Some(Bare { table: "test" })], functional_dependencies: FunctionalDependencies { deps: [] } }
However, the schema before is DFSchema { inner: Schema { fields: [Field { name: "col_int32", data_type: Int32, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [Some(Bare { table: "a" })], functional_dependencies: FunctionalDependencies { deps: [] } }
The only difference is field_qualifiers, but assert_schema_is_the_same will throw an error if schema changed during the optimization. 🤔
There was a problem hiding this comment.
I think a solution may be, when creating the schema for a plan and for Alias{expr, name}, we use the expr's name rather than the name, however this would also be a big changing I think. 🥲
There was a problem hiding this comment.
I would have thought that Union required all the inputs to have the exact same schema (and be properly coerced / aliased to make that the case)
Maybe there is a bug when constructing Union somewhere that permits the inputs to have different schemas 🤔
There was a problem hiding this comment.
Oh, I see. It seems true.
The current code will create the Union schema based on it's first input,
datafusion/datafusion/expr/src/logical_plan/builder.rs
Lines 1375 to 1400 in 1d4b808
However I think this example may be a use case, based on the example a Union has two Project plan as inputs, how to coerce them seems a question for me. 🥲 They already have different schemas and I can't know which one would be the merged schema.
There was a problem hiding this comment.
Or from earlier, what if when we build the schema for Project and the input for the Project is an Alias, we use Alias's expr name rather than Alias's name. 🤔 I'm not sure whether it would break some criteria.
There was a problem hiding this comment.
🤔 maybe we also need add an alias for columns in the subsequent children so the column names match exactly 🤔
There was a problem hiding this comment.
😢That's true. I'm kind of confusing how to deal with it.
There was a problem hiding this comment.
It looks like LogicalPlanBuilder::union has the code to do the schema coercion / matching
datafusion/datafusion/expr/src/logical_plan/builder.rs
Lines 1365 to 1429 in 405a5f6
So perhaps we need to either:
- Fix the
LogicalPlanBuildercode - Update optimizer passes to not create
LogicalPlan::Uniondirectly but instead use the builder (and thus apply the schema rationalization to occur)
There was a problem hiding this comment.
Yes, I understand you.
But for solution 1, like I said in #10494 (comment), the merged schema would use the left_qualifier in line 1395 - 1396, i.e. the first LogicalPlan's.
left_qualifier.cloned(),
Arc::new(Field::new(left_field.name(), data_type, nullable)),
However, I'm not quiet clear how to set the correct qualifier, they are different due to a SubqueryAlias🤔. Choosing either one would make some case failed. 😞
|
Marking as draft as I think this PR is no longer waiting on feedback. |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?