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

cloud-fan · 2021-07-05T08:07:19Z

What changes were proposed in this pull request?

This is a followup of #33113, to do some code cleanup:

UnresolvedFieldPosition doesn't need to include the field name. We can get it through "context" (AlterTableAlterColumn.column.name).
Run ResolveAlterTableCommands in the main resolution batch, so that the column/field resolution is also unified between v1 and v2 commands (same error message).
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

cloud-fan · 2021-07-05T08:07:28Z

cc @imback82

SparkQA · 2021-07-05T08:17:03Z

Test build #140642 has finished for PR 33213 at commit 34bd690.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedFieldPosition(position: ColumnPosition) extends FieldPosition

cloud-fan · 2021-07-05T08:51:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/StructType.scala

This is to match https://github.com/apache/spark/pull/33213/files#diff-583171e935b2dc349378063a5841c5b98b30a2d57ac3743a9eccfe7bffcb8f2aL472

SparkQA · 2021-07-05T09:05:08Z

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

SparkQA · 2021-07-05T09:40:09Z

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

SparkQA · 2021-07-05T09:44:39Z

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

SparkQA · 2021-07-05T10:18:27Z

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

SparkQA · 2021-07-05T11:00:24Z

Test build #140645 has finished for PR 33213 at commit 5246630.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedFieldPosition(position: ColumnPosition) extends FieldPosition

SparkQA · 2021-07-05T15:37:54Z

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

SparkQA · 2021-07-05T15:52:05Z

Test build #140660 has finished for PR 33213 at commit c31f8ba.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedFieldPosition(position: ColumnPosition) extends FieldPosition

SparkQA · 2021-07-05T16:12:21Z

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

imback82

LGTM (if tests pass), thanks @cloud-fan!

imback82 · 2021-07-05T17:27:32Z

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

-                  case o => alter.failAnalysis(s"Cannot ${alter.operation} ${fieldName.quoted}, " +
-                    s"because its parent is not a StructType. Found $o")
-                }
-                alter.failAnalysis(s"Couldn't resolve positional argument $position amongst " +


UnresolvedFieldPosition was carrying fieldName to reconstruct this message (no need to match by commands). If this is not needed anymore, we can drop fieldName. Thanks for cleaning this up.

The error message can refer to a position in the SQL string, which should provide sufficient context. The missing top-level column also uses a simple error message.

imback82 · 2021-07-05T17:31:12Z

...rc/test/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCTableCatalogSuite.scala

        sql(s"ALTER TABLE $tableName ALTER COLUMN bad_column COMMENT 'test'")
      }.getMessage
-      assert(msg.contains("Cannot update missing field bad_column in h2.test.alt_table"))
+      assert(msg.contains("Missing field bad_column in table h2.test.alt_table"))


If we are simplifying the message, we may not need to have AlterTableComand.operation. I can remove it in #33200.

sounds good, thanks!

SparkQA · 2021-07-05T18:02:34Z

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

SparkQA · 2021-07-05T18:36:04Z

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

cloud-fan · 2021-07-05T19:43:40Z

thanks for the review, merging to master/3.2 (since it's a followup).

### 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>

SparkQA · 2021-07-05T21:33:08Z

Test build #140668 has finished for PR 33213 at commit 52383ba.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

github-actions bot added the SQL label Jul 5, 2021

cloud-fan mentioned this pull request Jul 5, 2021

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

Closed

cloud-fan force-pushed the follow branch from 34bd690 to 5246630 Compare July 5, 2021 08:50

cloud-fan commented Jul 5, 2021

View reviewed changes

more code cleanup

c31f8ba

cloud-fan force-pushed the follow branch from 5246630 to c31f8ba Compare July 5, 2021 14:46

fix test

52383ba

imback82 approved these changes Jul 5, 2021

View reviewed changes

cloud-fan closed this in 8b46e26 Jul 5, 2021

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

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

Uh oh!

Conversation

cloud-fan commented Jul 5, 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!

cloud-fan commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

cloud-fan Jul 5, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

imback82 Jul 5, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 5, 2021

Choose a reason for hiding this comment

Uh oh!

imback82 Jul 5, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 5, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

cloud-fan commented Jul 5, 2021

Uh oh!

SparkQA commented Jul 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants