[SPARK-27857][SQL] Move ALTER TABLE parsing into Catalyst #24723

rdblue · 2019-05-27T22:09:18Z

What changes were proposed in this pull request?

This moves parsing logic for ALTER TABLE into Catalyst and adds parsed logical plans for alter table changes that use multi-part identifiers. This PR is similar to SPARK-27108, PR #24029, that created parsed logical plans for create and CTAS.

Create parsed logical plans
Move parsing logic into Catalyst's AstBuilder
Convert to DataSource plans in DataSourceResolution
Parse ALTER TABLE ... SET LOCATION ... separately from the partition variant
Parse ALTER TABLE ... ALTER COLUMN ... [TYPE dataType] [COMMENT comment] as discussed on the dev list
Parse ALTER TABLE ... RENAME COLUMN ... TO ...
Parse ALTER TABLE ... DROP COLUMNS ...

How was this patch tested?

Added new tests in Catalyst's DDLParserSuite
Moved converted plan tests from SQL DDLParserSuite to PlanResolutionSuite
Existing tests for regressions

SparkQA · 2019-05-27T23:31:06Z

Test build #105848 has finished for PR 24723 at commit 708fbdb.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class NewColumn(name: Seq[String], dataType: DataType, comment: Option[String])
case class AlterTableAddColumnsStatement(
case class AlterTableAlterColumnStatement(
case class AlterTableRenameColumnStatement(
case class AlterTableDropColumnsStatement(
case class AlterTableSetPropertiesStatement(
case class AlterTableUnsetPropertiesStatement(
case class AlterTableSetLocationStatement(
case class AlterViewSetPropertiesStatement(
case class AlterViewUnsetPropertiesStatement(

SparkQA · 2019-05-27T23:39:16Z

Test build #105849 has finished for PR 24723 at commit 36a2bcd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class NewColumn(name: Seq[String], dataType: DataType, comment: Option[String])
case class AlterTableAddColumnsStatement(
case class AlterTableAlterColumnStatement(
case class AlterTableRenameColumnStatement(
case class AlterTableDropColumnsStatement(
case class AlterTableSetPropertiesStatement(
case class AlterTableUnsetPropertiesStatement(
case class AlterTableSetLocationStatement(
case class AlterViewSetPropertiesStatement(
case class AlterViewUnsetPropertiesStatement(

rdblue · 2019-05-29T18:22:44Z

Retest this please.

SparkQA · 2019-05-29T19:43:21Z

Test build #105923 has finished for PR 24723 at commit 36a2bcd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class NewColumn(name: Seq[String], dataType: DataType, comment: Option[String])
case class AlterTableAddColumnsStatement(
case class AlterTableAlterColumnStatement(
case class AlterTableRenameColumnStatement(
case class AlterTableDropColumnsStatement(
case class AlterTableSetPropertiesStatement(
case class AlterTableUnsetPropertiesStatement(
case class AlterTableSetLocationStatement(
case class AlterViewSetPropertiesStatement(
case class AlterViewUnsetPropertiesStatement(

SparkQA · 2019-05-30T02:55:56Z

Test build #105934 has finished for PR 24723 at commit a4f56b8.

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

rdblue · 2019-05-30T16:26:06Z

@cloud-fan, @mccheah, @dongjoon-hyun, @jzhuge, could you review this PR? It updates SQL parsing for ALTER TABLE like the recent changes for CREATE TABLE and DROP TABLE. The parser is updated as discussed on the dev list.

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala

jzhuge

LGTM except just a few minors.

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

SparkQA · 2019-05-30T22:45:55Z

Test build #105975 has finished for PR 24723 at commit 84be6d2.

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

cloud-fan · 2019-05-31T12:47:30Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

Have you checked other databases/SQL standard that the parentheses can be omitted in the ALTER TABLE statement?

Yes. PostgreSQL, MySQL, and Oracle support this for a single column. PosgreSQL doesn't allow multiple columns. MySQL allows multiple columns without parens. And Oracle requires parens to add multiple columns. These don't assist parsing, which is why I think it is better to optionally support them.

Looks like DB2 and SQL server also allow adding multiple columns without parens.

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

SparkQA · 2019-06-01T00:33:01Z

Test build #106033 has finished for PR 24723 at commit 4902420.

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

SparkQA · 2019-06-01T03:15:46Z

Test build #106039 has finished for PR 24723 at commit 40c94dd.

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

cloud-fan · 2019-06-02T02:14:03Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

we should also add it to ansiNonReserved.

BTW is this a non-reserved keyword? @maropu

This is not an ANSI reserved keyword, but it is reserved in MySQL, PostgreSQL, and DB2. It is also on the list of potential ANSI reserved keywords.

cloud-fan · 2019-06-02T02:25:10Z

LGTM

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

gatorsmile · 2019-06-03T05:15:47Z

Normally, we check if it is possible to split it into multiple smaller, logical PRs or commits that can be quick to review.

Keep refactors in separate PRs
Fix one issue/bug per PR.

@ueshin Please help review this PR and ensure these DDLs are well tested, especially when handling nested columns.

SparkQA · 2019-06-03T19:59:13Z

Test build #106116 has finished for PR 24723 at commit 7e2369e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class QualifiedColType(name: Seq[String], dataType: DataType, comment: Option[String])

SparkQA · 2019-06-03T20:09:53Z

Test build #106117 has finished for PR 24723 at commit c558499.

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

ueshin · 2019-06-04T11:08:38Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4


 colPosition
-    : FIRST | AFTER identifier
+    : FIRST | AFTER qualifiedName


Do we need to modify this? I'm not sure what happens if something like:

ALTER TABLE tbl ADD a.b.c ... AFTER x.y

Yes, this is needed because columns are identified by qualifiedName. The parser shouldn't fail if a nested column name is used. Instead, analysis should catch problems.

I should also add that it is valid to reorder nested fields:

ALTER TABLE tbl ADD point.z bigint AFTER point.y

ueshin · 2019-06-04T11:11:36Z

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

+  override def visitAlterTableColumn(
+      ctx: AlterTableColumnContext): LogicalPlan = withOrigin(ctx) {
+    if (ctx.colPosition != null) {
+      operationNotAllowed("ALTER TABLE table CHANGE COLUMN ... FIRST | AFTER otherCol", ctx)


ALTER COLUMN instead of CHANGE COLUMN?

Both are supported, but CHANGE was supported first and is used in other places. I used CHANGE For consistency.

I'll update this to use the same verb that was parsed.

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

ueshin · 2019-06-04T11:25:23Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+        (ALTER | CHANGE) COLUMN? qualifiedName
+        (TYPE dataType)? (COMMENT comment=STRING)? colPosition?        #alterTableColumn
    | ALTER TABLE tableIdentifier partitionSpec?
        CHANGE COLUMN? identifier colType colPosition?                 #changeColumn


How will we handle this version of CHANGE COLUMN command?
Should we merge this in the (ALTER | CHANGE) COLUMN command above?

I think we need to discuss deprecating this form of the command, which is why I haven't updated it.

The problem is that this requires the user to specify too much of a column's metadata when it isn't changing. For example, if I'm updating an int to a long, I also need to specify that it should have the same name (UPDATE a a BIGINT). Similarly, to rename you have to pass the type back in (UPDATE a b INT). This can easily lead to unintended changes that can't be reverted, like widening a type accidentally.

I think that this form of the command should not be supported in v2. We can decide that later because all this is doing is updating the parser to add commands that we need to support.

ueshin · 2019-06-04T11:30:49Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala

+    case AlterTableAddColumnsStatement(AsTableIdentifier(table), newColumns)
+        if newColumns.forall(_.name.size == 1) =>
+      // only top-level adds are supported using AlterTableAddColumnsCommand
+      AlterTableAddColumnsCommand(table, newColumns.map(convertToStructField))


AlterTableAlterColumnStatement is missing?

Nothing implements AlterTableAlterColumnStatement. This PR preserves existing behavior and updates the parser, it does not add new behavior.

ueshin · 2019-06-04T13:12:21Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala

-    comparePlans(parsed2, expected2)
-  }
-
-  test("alter table: change column name/type/comment") {


This test should be preserved in PlanResolutionSuite?

Good catch, looks like I missed this one.

The parser didn't change for this variant of CHANGE COLUMN, so I'm adding it back. Looks like I just removed it accidentally.

rdblue · 2019-06-04T19:54:00Z

Thanks for reviewing this, @ueshin. I've addressed the problems you found. Please have another look.

SparkQA · 2019-06-04T22:09:37Z

Test build #106163 has finished for PR 24723 at commit bd7d8c8.

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

ueshin · 2019-06-05T12:50:16Z

LGTM.

rdblue · 2019-06-05T16:19:15Z

@dongjoon-hyun, could you have another look and possibly merge? I think the review items have been addressed. Thank you!

rdblue · 2019-06-05T16:19:42Z

Thanks for reviewing this, @ueshin!

gatorsmile · 2019-06-05T20:20:42Z

cc @yeshengm This PR will affect your parser changes. I will merge this PR first and you can address the issues in your PR.

gatorsmile · 2019-06-05T20:22:23Z

Thanks! Merged to master.

rdblue · 2019-06-05T22:17:49Z

Thank you for merging this, @gatorsmile!

This moves parsing logic for `ALTER TABLE` into Catalyst and adds parsed logical plans for alter table changes that use multi-part identifiers. This PR is similar to SPARK-27108, PR apache#24029, that created parsed logical plans for create and CTAS. * Create parsed logical plans * Move parsing logic into Catalyst's AstBuilder * Convert to DataSource plans in DataSourceResolution * Parse `ALTER TABLE ... SET LOCATION ...` separately from the partition variant * Parse `ALTER TABLE ... ALTER COLUMN ... [TYPE dataType] [COMMENT comment]` [as discussed on the dev list](http://apache-spark-developers-list.1001551.n3.nabble.com/DISCUSS-Syntax-for-table-DDL-td25197.html#a25270) * Parse `ALTER TABLE ... RENAME COLUMN ... TO ...` * Parse `ALTER TABLE ... DROP COLUMNS ...` * Added new tests in Catalyst's `DDLParserSuite` * Moved converted plan tests from SQL `DDLParserSuite` to `PlanResolutionSuite` * Existing tests for regressions Closes apache#24723 from rdblue/SPARK-27857-add-alter-table-statements-in-catalyst. Authored-by: Ryan Blue <[email protected]> Signed-off-by: gatorsmile <[email protected]>

## What changes were proposed in this pull request? This moves parsing logic for `ALTER TABLE` into Catalyst and adds parsed logical plans for alter table changes that use multi-part identifiers. This PR is similar to SPARK-27108, PR apache#24029, that created parsed logical plans for create and CTAS. * Create parsed logical plans * Move parsing logic into Catalyst's AstBuilder * Convert to DataSource plans in DataSourceResolution * Parse `ALTER TABLE ... SET LOCATION ...` separately from the partition variant * Parse `ALTER TABLE ... ALTER COLUMN ... [TYPE dataType] [COMMENT comment]` [as discussed on the dev list](http://apache-spark-developers-list.1001551.n3.nabble.com/DISCUSS-Syntax-for-table-DDL-td25197.html#a25270) * Parse `ALTER TABLE ... RENAME COLUMN ... TO ...` * Parse `ALTER TABLE ... DROP COLUMNS ...` ## How was this patch tested? * Added new tests in Catalyst's `DDLParserSuite` * Moved converted plan tests from SQL `DDLParserSuite` to `PlanResolutionSuite` * Existing tests for regressions Closes apache#24723 from rdblue/SPARK-27857-add-alter-table-statements-in-catalyst. Authored-by: Ryan Blue <[email protected]> Signed-off-by: gatorsmile <[email protected]>

rdblue changed the title ~~[SPARK-27857] Move ALTER TABLE parsing into Catalyst~~ [SPARK-27857][SQL] Move ALTER TABLE parsing into Catalyst May 27, 2019

rdblue force-pushed the SPARK-27857-add-alter-table-statements-in-catalyst branch from 708fbdb to 36a2bcd Compare May 27, 2019 22:18

jzhuge reviewed May 30, 2019

View reviewed changes

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

rdblue force-pushed the SPARK-27857-add-alter-table-statements-in-catalyst branch from a4f56b8 to 84be6d2 Compare May 30, 2019 19:41

jzhuge reviewed May 30, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala Outdated Show resolved Hide resolved

jzhuge reviewed May 30, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala Outdated Show resolved Hide resolved

jzhuge approved these changes May 30, 2019

View reviewed changes

dongjoon-hyun reviewed May 30, 2019

View reviewed changes

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

cloud-fan reviewed May 31, 2019

View reviewed changes

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 2, 2019

View reviewed changes

dongjoon-hyun reviewed Jun 2, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jun 2, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala Outdated Show resolved Hide resolved

gatorsmile reviewed Jun 3, 2019

View reviewed changes

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Outdated Show resolved Hide resolved

rdblue added 5 commits June 3, 2019 09:48

Add ALTER TABLE statements to Catalyst.

62c161e

Fixup char type when converting to v1 ADD COLUMN plan.

98bac21

Update for review comments.

d9694ba

Update missed qualifiedName.

5aa3b60

Fix ADD COLUMN with column position.

7e2369e

rdblue force-pushed the SPARK-27857-add-alter-table-statements-in-catalyst branch from f6c9005 to 7e2369e Compare June 3, 2019 16:49

rdblue added 2 commits June 3, 2019 09:55

Add convertToStructField for QualifiedColType.

1a3810a

Update name.

c558499

ueshin reviewed Jun 4, 2019

View reviewed changes

Fix issues from reviews.

bd7d8c8

gatorsmile closed this in 5d6758c Jun 5, 2019

rdblue deleted the SPARK-27857-add-alter-table-statements-in-catalyst branch June 21, 2019 18:42

dongjoon-hyun added the SQL label Feb 5, 2020

[SPARK-27857][SQL] Move ALTER TABLE parsing into Catalyst #24723

[SPARK-27857][SQL] Move ALTER TABLE parsing into Catalyst #24723

Uh oh!

Conversation

rdblue commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 27, 2019

Uh oh!

SparkQA commented May 27, 2019

Uh oh!

rdblue commented May 29, 2019

Uh oh!

SparkQA commented May 29, 2019

Uh oh!

SparkQA commented May 30, 2019

Uh oh!

rdblue commented May 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jzhuge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented May 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue May 31, 2019 • 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!

SparkQA commented Jun 1, 2019

Uh oh!

SparkQA commented Jun 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gatorsmile commented Jun 3, 2019

Uh oh!

SparkQA commented Jun 3, 2019

Uh oh!

SparkQA commented Jun 3, 2019

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!

Choose a reason for hiding this comment

Uh oh!

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

rdblue commented May 27, 2019 •

edited

Loading

rdblue May 31, 2019 •

edited

Loading