spark-test commented on code in PR #59919:
URL: https://github.com/apache/spark/pull/59919#discussion_r4415979761
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSchemaEvolution.scala:
##########
@@ -111,6 +113,27 @@ object ResolveSchemaEvolution extends Rule[LogicalPlan] {
plan.rewriteAttrs(attrMap).asInstanceOf[T]
}
+ /**
+ * Rewrites references to the metadata columns of the old relation (e.g. in
MERGE conditions)
+ * to the metadata columns of the new relation, which exposes them with new
expression IDs.
+ * Metadata columns are matched by their logical name, which stays the same
even if a data column
+ * of the new table renames a conflicting metadata column. References to
metadata columns that
+ * the new relation does not expose are left unchanged.
+ */
+ private def rewriteMetadataAttrs[T <: LogicalPlan](
+ plan: T,
+ oldRelation: DataSourceV2Relation,
+ newRelation: DataSourceV2Relation): T = {
+ val newMetadataAttrs: Map[String, Attribute] =
newRelation.metadataOutput.collect {
+ case MetadataAttributeWithLogicalName(attr, name) => name -> attr
+ }.toMap
+ val attrMap = AttributeMap(oldRelation.metadataOutput.collect {
Review Comment:
**Nit (P3):** This lookup also has to work when `oldRelation.metadataOutput`
comes from `output`. When evolution only widens a type (for example `salary`
INT to BIGINT with `UPDATE SET *`), every assignment key resolves before
evolution. `AddMetadataColumns` therefore runs first and merges the target's
metadata attributes into the relation output, and
`metadataOutputWithOutConflicts` then returns them from there. The PR
description says this case failed before the fix too. However, all three new
tests add the top-level column `active`, which keeps the MERGE unresolved until
evolution, so none of them reaches this state. A change that broke only this
path, for example skipping metadata attributes that are already in the old
relation's output, would bring back `MISSING_ATTRIBUTES` for those statements
while all five suites stay green. By code reading the current code handles the
case correctly, so this is about regression protection. A type-widening case
with a `t._partition` condit
ion in `MergeIntoSchemaEvolutionTypeWideningTests` would cover it in every
suite.
**Verification:**
- **Regression:** When schema evolution only widens a column type, a WHEN
MATCHED condition on the target partition metadata column updates exactly the
matching rows, leaves the others unchanged, and widens the table column. This
holds for group-based and delta-based operations through both the SQL and the
DataFrame merge APIs, and for delta-based operations that split updates through
the SQL API, while the same statement without the evolution clause keeps its
existing non-evolved outcome.
- **Regression:** With the metadata-column remap restricted to metadata
attributes that are not yet part of the old relation's output, the new case
fails analysis with the missing-attribute error while the existing
top-level-column cases still pass. This shows that the new case reaches the
state where the target metadata columns were already part of the relation
output before evolution; removing the remap entirely fails both kinds of case
and does not tell them apart.
--
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]
