[SPARK-34302][SQL] Migrate ALTER TABLE ... CHANGE COLUMN command to use UnresolvedTable to resolve the identifier #33113

imback82 · 2021-06-28T03:54:21Z

What changes were proposed in this pull request?

This PR proposes to migrate the following ALTER TABLE ... CHANGE COLUMN command to use UnresolvedTable as a child to resolve the table identifier. This allows consistent resolution rules (temp view first, etc.) to be applied for both v1/v2 commands. More info about the consistent resolution rule proposal can be found in JIRA or proposal doc.

Why are the changes needed?

This is a part of effort to make the relation lookup behavior consistent: SPARK-29900.

Does this PR introduce any user-facing change?

After this PR, the above ALTER TABLE ... CHANGE COLUMN commands will have a consistent resolution behavior.

How was this patch tested?

Updated existing tests.

imback82 · 2021-06-28T03:55:57Z

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

-                update.newDataType match {
-                  case _: StructType =>
-                    alter.failAnalysis(s"Cannot update ${table.name} field $fieldName type: " +
-                      s"update a struct by updating its fields")
-                  case _: MapType =>
-                    alter.failAnalysis(s"Cannot update ${table.name} field $fieldName type: " +
-                      s"update a map by updating $fieldName.key or $fieldName.value")
-                  case _: ArrayType =>
-                    alter.failAnalysis(s"Cannot update ${table.name} field $fieldName type: " +
-                      s"update the element by updating $fieldName.element")
-                  case u: UserDefinedType[_] =>
-                    alter.failAnalysis(s"Cannot update ${table.name} field $fieldName type: " +
-                      s"update a UserDefinedType[${u.sql}] by updating its fields")
-                  case _: CalendarIntervalType | _: YearMonthIntervalType |
-                       _: DayTimeIntervalType =>
-                    alter.failAnalysis(s"Cannot update ${table.name} field $fieldName to " +
-                      s"interval type")
-                  case _ =>
-                    // update is okay
-                }
-
-                // We don't need to handle nested types here which shall fail before


These changes are moved below to checkAlterTableCommand.

imback82 · 2021-06-28T03:59:13Z

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

-                positionArgumentExists(
-                  updatePos.position(),
-                  parent,
-                  colsToAdd.getOrElse(parentName, Nil))


I don't think we should consider colsToAdd since the CHANGE COLUMN command doesn't add a new column. So, in this PR, I didn't consider new columns added. Please let me know if I understood this wrong.

imback82 · 2021-06-28T04:05:09Z

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

+              }
+              field.get._2
+            }
+            def findParentStruct(fieldNames: Seq[String]): StructType = {


findField and findParentStruct are existing functions in CheckAnalysis with a slight modification (e.g., not passing operation argument). The existing functions will be removed once all alter table commands are migrated.

imback82 · 2021-06-28T05:11:31Z

cc @cloud-fan

cloud-fan · 2021-06-28T05:59:03Z

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

 }
+
+/**
+ * The logical plan of the ALTER TABLE ... CHANGE COLUMN command.


nit: let's put the SQL standard syntax in the doc ALTER TABLE ... ALTER COLUMN

cloud-fan · 2021-06-29T02:53:03Z

thanks, merging to master!

dongjoon-hyun · 2021-06-29T05:10:59Z

This seems to break JDBC v2 suite. I made a follow-up. Could you review that, @imback82 and @cloud-fan ?

[SPARK-34302][FOLLOWUP][SQL][TESTS] Update jdbc.v2.*IntegrationSuite #33124

SparkQA · 2021-06-29T20:14:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44892/

SparkQA · 2021-06-29T21:04:47Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44892/

cloud-fan · 2021-07-05T04:44:33Z

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

+            .map(dt => col.field.copy(dataType = dt))
+            .getOrElse(col.field)
+          val newDataType = a.dataType.get
+          newDataType match {


Not related to this PR: I think there is a small bug here, if the data type is not changed, we shouldn't fail even if the new data type is struct/array/map. @imback82 can you help to fix it?

Actually, this is OK. In the analzer, we will set the newDataType to None if it's the same with the existing data type in the table.

cloud-fan · 2021-07-05T05:17:43Z

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

+            resolveFieldNames(table.schema, u.name).getOrElse(u)
+          case u: UnresolvedFieldPosition => u.position match {
+            case after: After =>
+              resolveFieldNames(table.schema, u.fieldName.init :+ after.column())


Do we need to put fieldName in UnresolvedFieldPosition? We can easily get it via AlterTableAlterColumn.column.name

### What changes were proposed in this pull request? This is a followup of #33113, to do some code cleanup: 1. `UnresolvedFieldPosition` doesn't need to include the field name. We can get it through "context" (`AlterTableAlterColumn.column.name`). 2. Run `ResolveAlterTableCommands` in the main resolution batch, so that the column/field resolution is also unified between v1 and v2 commands (same error message). 3. Fail immediately in `ResolveAlterTableCommands` if we can't resolve the field, instead of waiting until `CheckAnalysis`. We don't expect other rules to resolve fields in ALTER TABLE commands, so failing immediately is simpler and we can remove duplicated code in `CheckAnalysis`. ### Why are the changes needed? code simplification. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests Closes #33213 from cloud-fan/follow. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? This is a followup of #33113, to do some code cleanup: 1. `UnresolvedFieldPosition` doesn't need to include the field name. We can get it through "context" (`AlterTableAlterColumn.column.name`). 2. Run `ResolveAlterTableCommands` in the main resolution batch, so that the column/field resolution is also unified between v1 and v2 commands (same error message). 3. Fail immediately in `ResolveAlterTableCommands` if we can't resolve the field, instead of waiting until `CheckAnalysis`. We don't expect other rules to resolve fields in ALTER TABLE commands, so failing immediately is simpler and we can remove duplicated code in `CheckAnalysis`. ### Why are the changes needed? code simplification. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests Closes #33213 from cloud-fan/follow. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 8b46e26) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

imback82 added 4 commits June 27, 2021 01:38

initial commit

ce47f89

more changes

2b908f8

update tests

727aa78

change rule name

a19f7b4

github-actions bot added the SQL label Jun 28, 2021

imback82 added 3 commits June 27, 2021 21:00

fix naming

4a97b36

add require message

808ab97

clean up test

0c1a5e1

imback82 commented Jun 28, 2021

View reviewed changes

cloud-fan reviewed Jun 28, 2021

View reviewed changes

Update test && address PR comment

6a75c92

cloud-fan closed this in 620fde4 Jun 29, 2021

cloud-fan reviewed Jul 5, 2021

View reviewed changes

cloud-fan mentioned this pull request Jul 5, 2021

[SPARK-34302][SQL][FOLLOWUP] More code cleanup #33213

Closed

[SPARK-34302][SQL] Migrate ALTER TABLE ... CHANGE COLUMN command to use UnresolvedTable to resolve the identifier #33113

[SPARK-34302][SQL] Migrate ALTER TABLE ... CHANGE COLUMN command to use UnresolvedTable to resolve the identifier #33113

Uh oh!

Conversation

imback82 commented Jun 28, 2021

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?

Uh oh!

imback82 Jun 28, 2021 • 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!

imback82 commented Jun 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 29, 2021

Uh oh!

dongjoon-hyun commented Jun 29, 2021

Uh oh!

SparkQA commented Jun 29, 2021

Uh oh!

SparkQA commented Jun 29, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imback82 Jun 28, 2021 •

edited

Loading