[SPARK-30535][SQL] Migrate ALTER TABLE commands to the new framework #27243

imback82 · 2020-01-16T23:16:16Z

What changes were proposed in this pull request?

Use the new framework to resolve the ALTER TABLE commands.

This PR also refactors ALTER TABLE logical plans such that they extend a base class AlterTable. Each plan now implements def changes: Seq[TableChange] for any table change operations.

Additionally, UnresolvedV2Relation and its usage is completely removed.

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?

No

How was this patch tested?

Updated existing tests

imback82 · 2020-01-16T23:16:42Z

cc @cloud-fan

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

imback82 · 2020-01-16T23:25:15Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

+      }
+      AlterTable(
+        r,
+        typeChange.toSeq ++ nullabilityChange ++ commentChange ++ positionChange)


Should this logic of populating Seq[TableChange] reside this this file? Can't this go directly into AlterTableAlterColumn and have a trait for ALTER TABLE commands which exposes (def changes: Seq[TableChage])?

yea sounds good! I think we can even make the logical plan AlterTable a base class for these ALTER TABLE commands, to make the workflow simpler: "different AlterTable logical plans -> AlterTableExec" instead of "different AlterTable logical plans -> AlterTable -> AlterTableExec"

SparkQA · 2020-01-17T01:13:01Z

Test build #116873 has finished for PR 27243 at commit ac12efc.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterTableAddColumns(
case class AlterTableAlterColumn(
case class AlterTableRenameColumn(
case class AlterTableDropColumns(
case class AlterTableSetProperties(
case class AlterTableUnsetProperties(
case class AlterTableSetLocation(

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

### What changes were proposed in this pull request? Use the new framework to resolve the SHOW TBLPROPERTIES command. This PR along with #27243 should update all the existing V2 commands with `UnresolvedV2Relation`. ### Why are the changes needed? This is a part of effort to make the relation lookup behavior consistent: [SPARK-2990](https://issues.apache.org/jira/browse/SPARK-29900). ### Does this PR introduce any user-facing change? Yes `SHOW TBLPROPERTIES temp_view` now fails with `AnalysisException` will be thrown with a message `temp_view is a temp view not table`. Previously, it was returning empty row. ### How was this patch tested? Existing tests Closes #26921 from imback82/consistnet_v2command. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

SparkQA · 2020-01-19T20:58:32Z

Test build #117017 has finished for PR 27243 at commit 81d692b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedTableWithViewExists(view: ResolvedView) extends LeafNode
case class ResolvedView(identifier: Identifier, isTempView: Boolean) extends LeafNode
abstract class AlterTable extends Command

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

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

SparkQA · 2020-01-20T01:48:34Z

Test build #117025 has finished for PR 27243 at commit 7a3f87f.

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

cloud-fan · 2020-01-20T06:35:20Z

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

+
+      case alter: AlterTable =>
+        alter.table match {
+          case u @ UnresolvedTableWithViewExists(view) if !view.isTempView =>


what's the error message for temp view?

The current/existing tests expect [t] is a temp view not a table.

shall we use the same error message as permanent views?

OK. I will do this as a follow-up. Thanks @cloud-fan for the review!

cloud-fan · 2020-01-20T06:37:54Z

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

@@ -420,63 +425,71 @@ trait CheckAnalysis extends PredicateHelper {
            }

          case alter: AlterTable if alter.childrenResolved =>


nit: case alter: AlterTable if alter.childrenResolved && alter.table.isInstanceOf[ResolvedTable]

Then we don't need to change the indentation below and save a lot of code diff.

BTW we can't hit v1 table here right? We should have converted alter table command to v1 if the table is v1, before we reach here.

Thanks, changed as suggested.

That's correct. If a table is V1Table, the alter table are converted to v1 in ResolveSessionCatalog.

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

cloud-fan · 2020-01-20T06:42:47Z

LGTM except a few minor comments

cloud-fan · 2020-01-20T07:40:48Z

LGTM. We can address #27243 (comment) in followup as it's only about error message.

SparkQA · 2020-01-20T08:05:02Z

Test build #117080 has finished for PR 27243 at commit 25ae584.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-20T08:05:02Z

Test build #117090 has finished for PR 27243 at commit bb21f16.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-01-20T08:31:13Z

retest this please

SparkQA · 2020-01-20T13:04:28Z

Test build #117105 has finished for PR 27243 at commit bb21f16.

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

cloud-fan · 2020-01-20T13:33:56Z

thanks, merging to master!

brkyvz · 2020-01-22T20:22:30Z

Sorry, but couldn't we have done this change after 3.0? Was there a bug around views with Alter Table to require us to the new resolution framework (for which we didn't have a proper discussion around the design of something so critical)? This seems too risky of a PR for several reasons:

It makes the resolution of the table that's going to be altered a child. We had avoided this on purpose as having an arbitrary rule change the child of AlterTable seemed risky. This change alone is a big -1 for me for this change.
While the code may look cleaner, I think this approach makes certain things harder, e.g. differentiating between the Hive based Alter table CHANGE COLUMN and ALTER COLUMN syntax. Resolving and normalizing columns for ALTER COLUMN also becomes a bit harder, as we now have to check every single AlterTable command instead of just a single ALTER TABLE ALTER COLUMN statement

gatorsmile · 2020-01-22T20:27:36Z

+1 Let us revert it now.

… framework ### What changes were proposed in this pull request? This reverts commit b5cb9ab. ### Why are the changes needed? The merged commit (#27243) was too risky for several reasons: 1. It doesn't fix a bug 2. It makes the resolution of the table that's going to be altered a child. We had avoided this on purpose as having an arbitrary rule change the child of AlterTable seemed risky. This change alone is a big -1 for me for this change. 3. While the code may look cleaner, I think this approach makes certain things harder, e.g. differentiating between the Hive based Alter table CHANGE COLUMN and ALTER COLUMN syntax. Resolving and normalizing columns for ALTER COLUMN also becomes a bit harder, as we now have to check every single AlterTable command instead of just a single ALTER TABLE ALTER COLUMN statement ### Does this PR introduce any user-facing change? No ### How was this patch tested? Existing unit tests This closes #27315 Closes #27327 from brkyvz/revAlter. Authored-by: Burak Yavuz <brkyvz@gmail.com> Signed-off-by: Xiao Li <gatorsmile@gmail.com>

gatorsmile · 2020-02-26T17:18:50Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

@@ -2194,7 +2194,7 @@ class DataSourceV2SQLSuite
    withTempView("v") {
      sql("create global temp view v as select 1")
      val e = intercept[AnalysisException](sql("COMMENT ON TABLE global_temp.v IS NULL"))


What happened if we apply this command to DSV1 tables? How many DDLs/DMLs are applied only to DSV2 tables?

I think all commands support v1 table: they either have a v1 command, or the v2 command works out of the box thanks to v2 session catalog. We need to add test though.

initial checkin

ac12efc

imback82 commented Jan 16, 2020

View reviewed changes

imback82 mentioned this pull request Jan 17, 2020

[SPARK-30282][SQL] Migrate SHOW TBLPROPERTIES to new framework #26921

Closed

imback82 commented Jan 17, 2020

View reviewed changes

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

test fixes

fc97bb5

imback82 added 2 commits January 17, 2020 08:42

Merge branch 'master' into v2commands_newframework

0f08bd2

address comments

81d692b

fix tests

7a3f87f

imback82 commented Jan 19, 2020

View reviewed changes

imback82 added 2 commits January 19, 2020 22:18

Address PR comments.

925fb54

minor refinement

25ae584

cloud-fan reviewed Jan 20, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala Outdated Show resolved Hide resolved

imback82 added 2 commits January 19, 2020 23:14

Address PR comments

20c5a61

minimize diff

bb21f16

cloud-fan closed this in b5cb9ab Jan 20, 2020

imback82 mentioned this pull request Jan 21, 2020

[SPARK-30535][SQL][FOLLOWUP] Update error message for ALTER TABLE when table is resolved to temp view #27315

Closed

brkyvz mentioned this pull request Jan 22, 2020

Revert "[SPARK-30535][SQL] Migrate ALTER TABLE commands to the new framework #27327

Closed

dongjoon-hyun added the SQL label Feb 5, 2020

gatorsmile reviewed Feb 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30535][SQL] Migrate ALTER TABLE commands to the new framework #27243

[SPARK-30535][SQL] Migrate ALTER TABLE commands to the new framework #27243

imback82 commented Jan 16, 2020 •

edited

imback82 commented Jan 16, 2020

imback82 Jan 16, 2020

cloud-fan Jan 17, 2020 •

edited

SparkQA commented Jan 17, 2020

SparkQA commented Jan 19, 2020

SparkQA commented Jan 20, 2020

cloud-fan Jan 20, 2020

imback82 Jan 20, 2020

cloud-fan Jan 20, 2020

imback82 Jan 20, 2020

cloud-fan Jan 20, 2020

cloud-fan Jan 20, 2020

imback82 Jan 20, 2020

cloud-fan commented Jan 20, 2020

cloud-fan commented Jan 20, 2020

SparkQA commented Jan 20, 2020

SparkQA commented Jan 20, 2020

cloud-fan commented Jan 20, 2020

SparkQA commented Jan 20, 2020

cloud-fan commented Jan 20, 2020

brkyvz commented Jan 22, 2020

gatorsmile commented Jan 22, 2020

gatorsmile Feb 26, 2020

cloud-fan Feb 26, 2020

		@@ -420,63 +425,71 @@ trait CheckAnalysis extends PredicateHelper {
		}

		case alter: AlterTable if alter.childrenResolved =>

[SPARK-30535][SQL] Migrate ALTER TABLE commands to the new framework #27243

[SPARK-30535][SQL] Migrate ALTER TABLE commands to the new framework #27243

Conversation

imback82 commented Jan 16, 2020 • edited

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?

imback82 commented Jan 16, 2020

Choose a reason for hiding this comment

cloud-fan Jan 17, 2020 • edited

Choose a reason for hiding this comment

SparkQA commented Jan 17, 2020

SparkQA commented Jan 19, 2020

SparkQA commented Jan 20, 2020

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

cloud-fan commented Jan 20, 2020

cloud-fan commented Jan 20, 2020

SparkQA commented Jan 20, 2020

SparkQA commented Jan 20, 2020

cloud-fan commented Jan 20, 2020

SparkQA commented Jan 20, 2020

cloud-fan commented Jan 20, 2020

brkyvz commented Jan 22, 2020

gatorsmile commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imback82 commented Jan 16, 2020 •

edited

cloud-fan Jan 17, 2020 •

edited