[SPARK-54621][SQL] Merge Into Update Set * preserve nested fields if …coerceNestedTypes is enabled by szehon-ho · Pull Request #53360 · apache/spark

szehon-ho · 2025-12-06T00:46:18Z

What changes were proposed in this pull request?

The 'struct coercion' feature for MERGE INTO (allowing it to pass if assigning a struct with less fields into a struct with more fields) is turned off in a flag in #53229 due to some ambiguity in behavior, but was not removed because the community wanted to try it.

We want to still keep it under a flag, but we make a choice about which behavior to support when the flag is on. In particular, we want UPDATE SET * to explode to all nested struct fields, so that in this scenario, existing nested struct fields are preserved.

Why are the changes needed?

@aokolnychyi tested the feature and thinks that even if it is behind the experimental flag, we should take the stance for now that UPDATE SET * should explode to all nested fields vs top level columns.

The rationale being:

its always safer to not override user values with null
Spark in general tries to treat nested fields like columns
there's already a way for the user to override the whole struct (and nullify non-existing fields) by specifying the struct explicitly, ie UPDATE SET struct = source.struct

Does this PR introduce any user-facing change?

No, the whole feature is new and hidden behind an experimental flag.

How was this patch tested?

Existing tests (some output changes to not be null)

Was this patch authored or co-authored using generative AI tooling?

No

szehon-ho · 2025-12-06T00:47:58Z

Hi @dongjoon-hyun sorry for the back and forth here. As in the description, @aokolnychyi explained he preferred to make the behavior choose UPDATE SET * to refer to nested fields, due to the reasons above. The whole feature (MERGE INTO struct coercion) is still under an experimental flag and off by default, but we want to make this stance if the flag is on.

szehon-ho · 2025-12-06T00:48:41Z

Btw, The code is not new code, its the same code in #53149 which was removed, it is just brought back.

dongjoon-hyun · 2025-12-06T00:53:44Z

No problem, but I believe this is only applicable for master branch only, @szehon-ho .

dongjoon-hyun · 2025-12-06T00:54:13Z

So, +1 for 4.2.0 for the proposal although I didn't take a look at the code yet.

szehon-ho · 2025-12-06T05:54:39Z

Hi, @dongjoon-hyun , @aokolnychyi mentioned it would be good to get into 4.1, because we are still releasing the feature of 'struct coercion' , albeit with a flag. So he wanted to start it off with the better choice. Code-wise its the same as before the revert, although the whole thing has a flag. Seems from the comments of #53229, the community is interested in this feature. Ill ping him to comment as well

…coerceNestedTypes is enabled

dongjoon-hyun · 2025-12-09T16:08:02Z

Sorry but I still believe this fits for Apache Spark 4.2.0 (after checking the code again). This is only for Apache Spark 4.2.0, @szehon-ho . We are ramping down instead of ramping up.

…ll OR target has extra fields)

szehon-ho · 2025-12-12T06:25:59Z

+    }
+  }
+
+  private def applyNestedFieldAssignments(


note: this is like applyFieldAssignment above, but recurses to all nested fields

applyFieldAssignments is not recursive? what's its behavior?

iiuc, it is not , it just looks at missing assignments for the first level of schema and does some validation (like no two assignments to same field), and then uses TableOutputResolver to fill missing ones.

i just chatted with @aokolnychyi on the difference, and wanted to clarify here as well. I mean, the existing method applyFieldAssignments is obviously recursive, but it's goal is not to make assignment for every nested field, but just missing siblings at the existing assignment levels. For example, we have update x.y.z = source.x.y.z. It will recurse down to evaluate x.y struct, see the assignment for x.y.z. It will fill missing assignments for z's siblings, ie if there was x.y.z1 and x.y.z2 it will fill those

The new method on the other hand 'explodes' an assignment for the top level struct x (if its from UPDATE SET *) into assignment for every single field, ie not only the missing siblings of existing assignments. Hope that is more clear on the difference.

cloud-fan · 2025-12-12T12:08:59Z

Does this change anything when MERGE_INTO_NESTED_TYPE_COERCION_ENABLED is false?

szehon-ho · 2025-12-12T17:31:25Z

Does this change anything when MERGE_INTO_NESTED_TYPE_COERCION_ENABLED is false?

Yea, it should not, that should be the guard for the whole feature (nested type coercion)

aokolnychyi · 2025-12-12T22:45:48Z

This PR actually fixes an issue I discovered while testing MERGE with 4.1 RC in Iceberg. I believe the current logic in Spark 4.1 is a regression and leads to data loss (we replace existing nested fields with nulls, for instance). Spark 4.0 was a lot stricter than 4.1 and some of the 4.1 behavior is invalid.

szehon-ho · 2025-12-14T21:51:33Z

Yes after chatting with @aokolnychyi this can be serious as it cause inadvertent data loss. For instance, previously the user doing MERGE has a source struct s with just one nested field, and the target struct s with 100 nested fields. The user doing UPDATE SET * in 4.0 will get a failure, and they will realize their mistake. But now that we relax it in 4.1, a user doing UPDATE SET * with schema evolution will lose the 99 fields not in source struct s (become null). We should instead retain their value.

cloud-fan · 2025-12-15T02:03:20Z

+    // As StoreAssignmentPolicy.LEGACY is not allowed in DSv2, always add null check for
+    // non-nullable column
+    if (!key.nullable) {
+      AssertNotNull(value)


This is useless. If we really want to do this optimization, we should add the AssertNotNull before we use the value to construct GetStructField.

And we can do this optimization in the else branch of the If expression as well.

I tried this change, i remember doing this change initially, but it seems its' a must to skip the IF (...) NULL for non-nullable fields.

In this line below,

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala

Line 246 in cc03f85

(attr.nullable || !assignment.value.nullable)

it checks that the assignment value is non-nullable just like the key is. If there is a IF (...) NULL (even if it never hits), then it returns false, and analyzer runs forever.

ok, then we should give up this optimization for now and return structExpression here?

Yea maybe we can fix the condition later, possibly? can discuss with @aokolnychyi

I may have mis-understood your message. I also removed the AssertNotNull

cloud-fan · 2025-12-15T02:05:33Z

+                case Some(matchingField) =>
+                  // Found matching field in source, extract it
+                  val fieldIndex = valueStructType.fieldIndex(matchingField.name)
+                  GetStructField(value, fieldIndex, Some(matchingField.name))


See https://github.com/apache/spark/pull/53360/files#r2617734660

This is where we can wrap value with AssertNotNull

dongjoon-hyun

I'll blocking this PR because we have a discussion the RC3 thread.

As I made a decision last week on this PR, I'm still very relunctant to have this code in 4.1.0, @szehon-ho , @cloud-fan , @aokolnychyi .

Please don't land this to branch-4.1..

Addressed.

dongjoon-hyun · 2025-12-18T07:31:35Z

I resolved my previous review comment. Thank you, @szehon-ho .

cloud-fan · 2025-12-18T07:42:46Z

thanks, merging to master/4.1!

……coerceNestedTypes is enabled ### What changes were proposed in this pull request? The 'struct coercion' feature for MERGE INTO (allowing it to pass if assigning a struct with less fields into a struct with more fields) is turned off in a flag in #53229 due to some ambiguity in behavior, but was not removed because the community wanted to try it. We want to still keep it under a flag, but we make a choice about which behavior to support when the flag is on. In particular, we want UPDATE SET * to explode to all nested struct fields, so that in this scenario, existing nested struct fields are preserved. ### Why are the changes needed? aokolnychyi tested the feature and thinks that even if it is behind the experimental flag, we should take the stance for now that UPDATE SET * should explode to all nested fields vs top level columns. The rationale being: * its always safer to not override user values with null * Spark in general tries to treat nested fields like columns * there's already a way for the user to override the whole struct (and nullify non-existing fields) by specifying the struct explicitly, ie UPDATE SET struct = source.struct ### Does this PR introduce _any_ user-facing change? No, the whole feature is new and hidden behind an experimental flag. ### How was this patch tested? Existing tests (some output changes to not be null) ### Was this patch authored or co-authored using generative AI tooling? No Closes #53360 from szehon-ho/SPARK-54621. Authored-by: Szehon Ho <szehon.apache@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 92e5b36) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

github-actions Bot added the SQL label Dec 6, 2025

szehon-ho force-pushed the SPARK-54621 branch from 1c554ef to 27f40d5 Compare December 6, 2025 05:53

[SPARK-54621][SQL] Merge Into Update Set * preserve nested fields if …

2040335

…coerceNestedTypes is enabled

szehon-ho force-pushed the SPARK-54621 branch from 27f40d5 to 2040335 Compare December 9, 2025 00:29

szehon-ho added 2 commits December 11, 2025 21:54

Fix the condition to preserve nulls (source is null AND (target is nu…

3485d55

…ll OR target has extra fields)

Simplification

810f6a5

szehon-ho commented Dec 12, 2025

View reviewed changes

cloud-fan reviewed Dec 12, 2025

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala Outdated

cloud-fan reviewed Dec 12, 2025

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala Outdated

cloud-fan reviewed Dec 12, 2025

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala Outdated

cloud-fan reviewed Dec 12, 2025

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala

Review comments

73aedae

szehon-ho force-pushed the SPARK-54621 branch from 0576f78 to 73aedae Compare December 14, 2025 06:02

cloud-fan reviewed Dec 15, 2025

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala

cloud-fan reviewed Dec 15, 2025

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentUtils.scala Outdated

cloud-fan reviewed Dec 15, 2025

View reviewed changes

cloud-fan approved these changes Dec 15, 2025

View reviewed changes

Remove unneeded variable

fd7efb2

dongjoon-hyun previously requested changes Dec 16, 2025

View reviewed changes

aokolnychyi mentioned this pull request Dec 16, 2025

[SPARK-54721][SQL] Completely disable feature 'nested struct coercion' for MERGE INTO #53490

Closed

Remove assertNotNull

961f285

szehon-ho force-pushed the SPARK-54621 branch from 4dc6e56 to 961f285 Compare December 17, 2025 20:10

cloud-fan closed this in 92e5b36 Dec 18, 2025

Conversation

szehon-ho commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

szehon-ho commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szehon-ho commented Dec 6, 2025

Uh oh!

dongjoon-hyun commented Dec 6, 2025

Uh oh!

dongjoon-hyun commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szehon-ho commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Dec 12, 2025

Uh oh!

szehon-ho commented Dec 12, 2025

Uh oh!

aokolnychyi commented Dec 12, 2025

Uh oh!

szehon-ho commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 18, 2025

Uh oh!

cloud-fan commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

szehon-ho commented Dec 6, 2025 •

edited

Loading

szehon-ho commented Dec 6, 2025 •

edited

Loading

dongjoon-hyun commented Dec 6, 2025 •

edited

Loading

szehon-ho commented Dec 6, 2025 •

edited

Loading

szehon-ho Dec 12, 2025 •

edited

Loading

szehon-ho commented Dec 14, 2025 •

edited

Loading

szehon-ho Dec 15, 2025 •

edited

Loading