[SPARK-56054][SQL] Undo handling aliased assignments in MERGE INTO schema evolution and add tests #55239
Conversation
szehon-ho
left a comment
There was a problem hiding this comment.
these are all DataFrame API, wondering is there any equivalent in SQL? Be good to abstract it out if possible
If not, maybe we can we make a new file MergeIntoSchemaEvolutionExtraDataFrameTests to make them only run in DataFrame mode?
It is a bit of a mess now due to the inheritance patterns
|
I moved the tests to a dedicated trait for dataframe tests. There's no SQL equivalent, it's not possible to specify an alias in an assignment expression using SQL afaict: |
| spark.table("source") | ||
| .mergeInto(tableNameAsString, | ||
| col(s"$tableNameAsString.pk") === col("source.pk")) | ||
| .whenMatched().update(Map("info" -> col("source.info").as("info"))) |
There was a problem hiding this comment.
Okay, this seems like a behavior change compared to the initial PR then? The code in master would evolve the schema in this case because we had that alias support?
There was a problem hiding this comment.
Correct. The main motivation for the initial PR adding support for alias was to support nested assignments SET col.x = s.col.x, which translates to Assignment("col.x", Alias(GetStructField("col", "x"), "x")
I've noticed in the meantime that Spark actually strips the alias on struct field access after resolution (Delta doesn't).
We don't have a strong argument to support the remaining cases where the user provides an explicit alias using the dataframe API, so I'd rather not allow these
| .mergeInto(tableNameAsString, | ||
| col(s"$tableNameAsString.pk") === col("source.pk")) | ||
| .whenMatched().update(Map( | ||
| "info" -> col("source.info").as("something_else"))) |
There was a problem hiding this comment.
How does Delta behave in this case?
There was a problem hiding this comment.
Delta allows schema evolution here, but only because it allows arbitrary expressions on the right hand side of the assignment. col("source.info").as("something_else") can qualifies for schema evolution the same way col("source.other_column").plus(lit(1)) does.
Spark rejects both (with this change)
|
@johanl-db, what about cases when the source query has an alias? |
Discussed directly with @aokolnychyi and @szehon-ho :
MERGE doesn't look inside the source query to see what it contains, so the aliases there are irrelevant. |
|
CI is failing due to formatting on files that are not touched in this PR. Likely from #55281 |
There was a problem hiding this comment.
I think this one is ready to go @cloud-fan
i also added the test requested by @aokolnychyi in #55304 (i didnt see the comment), we can add it separately , or optionally as part this pr, i guess it doesnt matter. As we looked into it together, its a bit orthogonal to this pr (the aliases within source doens't matter, to MERGE it just comes as a source table)
|
also , apparently this ci error is fixed by #55334 |
@szehon-ho I picked up your test from #55304 in this PR |
cloud-fan
left a comment
There was a problem hiding this comment.
Summary
This is a follow-up to #54891 that reverts the generic case Alias(child, _) branch in MergeIntoTable.extractFieldPath and expands the test surface. The rationale: the implicit trivial Alias(ExtractValue, _) that AttributeSeq.resolve adds around nested field accesses is already stripped by resolveExprInAssignment (ColumnResolutionHelper.scala:489) before values reach extractFieldPath. So nested assignments like SET target.info.b = source.info.b keep working (and are already guarded by existing tests in MergeIntoSchemaEvolutionBasicTests.scala:629/691). The generic Alias case in #54891 additionally picked up explicit user aliases on top-level struct columns (col("src.info").as("info")) — an unintended side-effect that isn't a pattern Spark core needs to support. Delta can (and does) handle that via its own custom resolution.
Net effect: alias handling for schema evolution is consolidated in the resolution layer; top-level explicit aliased struct columns revert to the pre-#54891 behavior (no evolution, incompatible-schema error). #54891 is on master only and unreleased, so no shipped behavior regresses.
The four new negative tests in MergeIntoSchemaEvolutionExtraScalaTests pin down the now-unsupported patterns (top-level aliased struct with matching/mismatched name, nested field through an aliased struct, complex expression value). The new SQL test covers the source-subquery-alias case raised by @aokolnychyi.
LGTM.
|
thanks, merging to master! |
What changes were proposed in this pull request?
Follow up from #54891 in particular this comment.
Adds more tests covering schema evolution in MERGE INTO when the assignment contains an alias.
This change also undoes the fix from #54891. On closer inspection, this isn't needed in Spark as Spark already removes trivial aliases on nested field accessors in MERGE in resolveExprInAssignment, which is what the change aimed to allow. See this test added here covering trivial aliases implictily added on nested field access.
Delta doesn't strip aliases during resolution in that way and will require special handling, but that's up to Delta to implement in its custom resolution logic to replicate what Spark already does
How was this patch tested?
Adds tests covering MERGE INTO schema evolution with aliases in assignments