[SPARK-52095][SQL] Alter table alter column to pass V2Expression to DSV2 #50864

szehon-ho · 2025-05-13T00:11:21Z

What changes were proposed in this pull request?

"Alter table alter column" to pass in V2 Expression to DSV2.
Like the similar changes (#50593) and (#50701), the existing logic is rewritten to use the main Analyzer loop to get this expression, instead of manual call to ResolveDefaultColumns to analyze.

We enhance the UpdateColumnValue (TableChanges API) to return DefaultValue (which contains the V2 Expression), in addition to the existing API returning String representation of the default value.

Why are the changes needed?

DSV2 (example, Iceberg/Delta) should get modeled V2 Expression to set default value.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added tests in DataSourceV2DataFrameSuite

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

No

szehon-ho · 2025-05-13T00:23:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala

@@ -233,4 +246,13 @@ case class DefaultValueExpression(
    case _ =>
      throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
  }
+
+  // Convert the default expression to DefaultValue, which is required by DS v2 APIs.
+  def toV2CurrentDefault(statement: String, colName: String): DefaultValue = child match {


"Alter table alter column" doesnt change 'exists default', so we can use the narrower 'DefaultValue' class here that doesnt have that field.

cloud-fan · 2025-05-13T17:52:40Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

        } else if (action.dropDefault != null) {
-          Some("")
+          Some(DefaultValueExpression(Literal(""), ""))


shall we use DefaultValueExpression(null, "") to indicate dropping default?

sure, I was initially thinking Literal("") to match "" that was used before, but that makes sense too

hm, i think in DefaultValueExpression, child is not Option, so its expected to be always set?

Cannot invoke "org.apache.spark.sql.catalyst.trees.TreeNode.exists(scala.Function1)" because "x$6" is null java.lang.NullPointerException: Cannot invoke "org.apache.spark.sql.catalyst.trees.TreeNode.exists(scala.Function1)" because "x$6" is null at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$exists$1(TreeNode.scala:240) at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$exists$1$adapted(TreeNode.scala:240) at scala.collection.IterableOnceOps.exists(IterableOnce.scala:647) at scala.collection.IterableOnceOps.exists$(IterableOnce.scala:644) at scala.collection.AbstractIterable.exists(Iterable.scala:935) at org.apache.spark.sql.catalyst.trees.TreeNode.exists(TreeNode.scala:240) at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$exists$1(TreeNode.scala:240) at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$exists$1$adapted(TreeNode.scala:240) at scala.collection.immutable.List.exists(List.scala:396) at org.apache.spark.sql.catalyst.trees.TreeNode.exists(TreeNode.scala:240) at org.apache.spark.sql.execution.QueryExecution.$anonfun$isLazyAnalysis$2(QueryExecution.scala:74) at org.apache.spark.sql.execution.QueryExecution.$anonfun$isLazyAnalysis$2$adapted(QueryExecution.scala:74) at scala.collection.immutable.List.exists(List.scala:396) at org.apache.spark.sql.execution.QueryExecution.$anonfun$isLazyAnalysis$1(QueryExecution.scala:74) at org.apache.spark.sql.execution.QueryExecution.$anonfun$isLazyAnalysis$1$adapted(QueryExecution.scala:74) at org.apache.spark.sql.catalyst.trees.TreeNode.exists(TreeNode.scala:237) at org.apache.spark.sql.execution.QueryExecution.isLazyAnalysis$lzycompute(QueryExecution.scala:74) at org.apache.spark.sql.execution.QueryExecution.isLazyAnalysis(QueryExecution.scala:72) at org.apache.spark.sql.classic.Dataset$.$anonfun$ofRows$5(Dataset.scala:139) at org.apache.spark.sql.SparkSession.withActive(SparkSession.scala:804) at org.apache.spark.sql.classic.Dataset$.ofRows(Dataset.scala:136) at org.apache.spark.sql.classic.SparkSession.$anonfun$sql$4(SparkSession.scala:570) at org.apache.spark.sql.SparkSession.withActive(SparkSession.scala:804)

oh, how about Literal(null)? My point is that Literal("") means an empty string literal, which is "''", not ""

cloud-fan · 2025-05-13T17:53:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala

@@ -176,6 +177,18 @@ object ColumnDefinition {
          }
        }

+      case cmd: AlterColumns if cmd.specs.exists(_.newDefaultExpression.isDefined) =>
+        // Wrap analysis errors for default values in a more user-friendly message.


not related to this PR but we can probably do the same error message improvement for V2CreateTablePlan

cloud-fan · 2025-05-13T17:58:29Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala

+  override protected def withNewChildrenInternal(
+      newChildren: IndexedSeq[Expression]): Expression = {
+    val newColumn = newChildren(0).asInstanceOf[FieldName]
+    val newPosition = newChildren collectFirst {


we can avoid instance checking

val newPos = if (newPosition.isDefined) { Some(newChildren(1).asInstanceOf[FieldPosition]) } else { None } val newDefault = if (newDefaultExpression.isDefined) { Some(newChildren.last.asInstanceOf[DefaultValueExpression]) } else { None }

nice idea, done

szehon-ho · 2025-05-14T18:57:48Z

also, latest change uses the new exception type added in #50879

aokolnychyi · 2025-05-15T00:29:47Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java

+     * is not provided as updating column default values does not need to back-fill existing data.
+     * Empty string and Null literal means dropping the column default value.
+     */
+    public DefaultValue newModelDefaultValue() { return newDefaultValue; }


Why not interpret null for DefaultValue as drop?

Looks like for original ALTER COLUMN, null string meant no default was set, it was different than "" meaning drop default.

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala#L5246

I was trying to be consistent with that, I could take a look, but it would be more code to check if we are "" and set null.

null string meant no default was set

Shouldn't we avoid generating UpdateColumnDefaultValue at all in that case?

yes thats the current mechanism here:

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala#L245

if its null, we dont make UpdateColumnDefaultValue. If drop , we make it with "". Else, we make it with the user-specified value.

@cloud-fan, do you by any chance remember the context on why we used "" instead of null?

Discussed a bit offline, we can probably change catalyst side and add a boolean/enum to AlterColumnSpec to indicate whether its Drop or Unchanged.

In DSV2 side, we should ideally have no UpdateColumnDefaultValue for unchanged, have UpdateColumnDefaultValue new API return null for Drop case, and have it return the default if there's actually one. Returning DefaultValue(Literal(null), "") is a bit hard for connectors

We will preserve the old UpdateColumnDefaultValue string API to return "" though for backward compatibility.

@cloud-fan if you have thoughts

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java

aokolnychyi · 2025-05-15T00:42:12Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java

+     * is not provided as updating column default values does not need to back-fill existing data.
+     * Empty string and Null literal means dropping the column default value.
+     */
+    public DefaultValue newModelDefaultValue() { return newDefaultValue; }


I am not sure I like the name here. I get that the ideal name newDefaultValue is already taken, but let's brainstorm a bit more. What about any of these?

newValue()

updatedDefaultValue()

newColumnDefaultValue()

newCurrentDefaultValue()

Any other suggestions, @szehon-ho @cloud-fan @gengliangwang?

yea agree its not the greatest name. Renamed it newCurrentDefault, let me know if strong opinion otherwise.

aokolnychyi · 2025-05-15T00:56:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

        } else if (action.dropDefault != null) {
-          Some("")
+          Some(DefaultValueExpression(Literal(null), ""))


This seems a bit fragile. I wonder whether there is a better way to handle it. We can have an enum or something with unchanged, drop, and set options. It only makes sense, however, if we want to pass null for DefaultValue to indicate drop in the public connector API.

Fixed in the latest pr, add a boolean dropDefault in AlterColumnSpec

cloud-fan · 2025-05-16T09:44:16Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java

@@ -726,22 +746,29 @@ public String[] fieldNames() {
     * is not provided as updating column default values does not need to back-fill existing data.
     * Empty string means dropping the column default value.
     */
-    public String newDefaultValue() { return newDefaultValue; }
+    public String newDefaultValue() {


shall we mark this function as deprecated and encourage users to call newCurrentDefault?

cloud-fan · 2025-05-16T09:46:14Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java

+     * is not provided as updating column default values does not need to back-fill existing data.
+     * Null means dropping the column default value.
+     */
+    public DefaultValue newCurrentDefault() { return newCurrentDefault; }

    @Override
    public boolean equals(Object o) {


did you generate equals and hashCode using IDEA?

yes, whats the recommendation?

ah sorry i used wrong template in Intellij, fixed

github-actions bot added the SQL label May 13, 2025

szehon-ho commented May 13, 2025

View reviewed changes

szehon-ho force-pushed the replace_col_default_val branch 2 times, most recently from 824bd0d to b96182e Compare May 13, 2025 05:38

cloud-fan reviewed May 13, 2025

View reviewed changes

cloud-fan approved these changes May 13, 2025

View reviewed changes

szehon-ho force-pushed the replace_col_default_val branch from 95a159e to 0341236 Compare May 14, 2025 18:55

szehon-ho force-pushed the replace_col_default_val branch from 0341236 to 492122f Compare May 14, 2025 19:01

szehon-ho added 4 commits May 14, 2025 14:15

Alter table alter column to pass V2Expression to DSV2

ba2accb

Lint

3539dd0

review comments

dbd29ee

Review comments

f53ecec

szehon-ho force-pushed the replace_col_default_val branch from 492122f to f53ecec Compare May 14, 2025 21:15

aokolnychyi reviewed May 15, 2025

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Show resolved Hide resolved

aokolnychyi reviewed May 15, 2025

View reviewed changes

szehon-ho added 3 commits May 14, 2025 18:04

Rename new method and regenerate equals/hashcode

0877f02

Properly model drop default with a boolean instead of using ""

e8ea5ad

Fix v1

e176c62

cloud-fan reviewed May 16, 2025

View reviewed changes

cloud-fan approved these changes May 16, 2025

View reviewed changes

szehon-ho added 2 commits May 16, 2025 10:12

Deprecate old method and regenerate equals/hashcode

ec5c930

Clarify a javadoc comment

d5a3b0c

cloud-fan approved these changes May 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-52095][SQL] Alter table alter column to pass V2Expression to DSV2 #50864

[SPARK-52095][SQL] Alter table alter column to pass V2Expression to DSV2 #50864

szehon-ho commented May 13, 2025

szehon-ho May 13, 2025

cloud-fan May 13, 2025

szehon-ho May 13, 2025

szehon-ho May 13, 2025 •

edited

Loading

cloud-fan May 14, 2025

szehon-ho May 14, 2025

cloud-fan May 13, 2025

cloud-fan May 13, 2025

szehon-ho May 13, 2025

szehon-ho commented May 14, 2025 •

edited

Loading

aokolnychyi May 15, 2025

szehon-ho May 15, 2025 •

edited

Loading

aokolnychyi May 15, 2025

szehon-ho May 15, 2025 •

edited

Loading

aokolnychyi May 15, 2025

szehon-ho May 15, 2025 •

edited

Loading

aokolnychyi May 15, 2025

szehon-ho May 15, 2025

aokolnychyi May 15, 2025

szehon-ho May 15, 2025

cloud-fan May 16, 2025

cloud-fan May 16, 2025

szehon-ho May 16, 2025

szehon-ho May 16, 2025 •

edited

Loading

[SPARK-52095][SQL] Alter table alter column to pass V2Expression to DSV2 #50864

Are you sure you want to change the base?

[SPARK-52095][SQL] Alter table alter column to pass V2Expression to DSV2 #50864

Conversation

szehon-ho commented May 13, 2025

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho May 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho commented May 14, 2025 • edited Loading

Choose a reason for hiding this comment

szehon-ho May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho May 16, 2025 • edited Loading

Choose a reason for hiding this comment

szehon-ho May 13, 2025 •

edited

Loading

szehon-ho commented May 14, 2025 •

edited

Loading

szehon-ho May 15, 2025 •

edited

Loading

szehon-ho May 15, 2025 •

edited

Loading

szehon-ho May 15, 2025 •

edited

Loading

szehon-ho May 16, 2025 •

edited

Loading