[SPARK-27813][SQL] DataSourceV2: Add DropTable logical operation #24686

jzhuge · 2019-05-23T09:12:49Z

What changes were proposed in this pull request?

Support DROP TABLE from V2 catalogs.
Move DROP TABLE into catalyst.
Move parsing tests for DROP TABLE/VIEW to PlanResolutionSuite to validate existing behavior.
Add new tests fo catalyst parser suite.
Separate DROP VIEW into different code path from DROP TABLE.
Move DROP VIEW into catalyst as a new operator.
Add a meaningful exception to indicate view is not currently supported in v2 catalog.

How was this patch tested?

New unit tests.
Existing unit tests in catalyst and sql core.

SparkQA · 2019-05-23T12:15:12Z

Test build #105716 has finished for PR 24686 at commit de0ab34.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class AstBuilder(conf: SQLConf)
case class DropTable(
case class DropTableExec(catalog: TableCatalog, ident: Identifier, ifExists: Boolean)

jzhuge · 2019-05-23T17:01:33Z

Split the SPARK-26946 commits into a separate PR #24689

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/IdentifierImpl.java

jzhuge · 2019-05-24T15:26:43Z

Making a few changes. Close it for now.

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

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalog/v2/TestTableCatalog.scala

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalog/v2/LookupCatalog.scala

Treat it as catalog not found.

SparkQA · 2019-05-27T01:42:28Z

Test build #105805 has finished for PR 24686 at commit db4a417.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class CatalogManager
\"Catalog '%s' plugin class not found: %s is not defined\", name, classKey(name)));
case class DropTable(
case class DropTableStatement(
case class DropTableExec(catalog: TableCatalog, ident: Identifier, ifExists: Boolean)

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/CatalogManager.java

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/Catalogs.java

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

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

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

…t defined" This reverts commit a79cb84

This reverts commit 7e11ca6

jzhuge · 2019-05-29T16:15:04Z

No longer depend on #24689.
Did a better job isolating the changes only needed for DropTable this time, so diff should be much more cleaner.

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

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

rdblue · 2019-05-29T18:38:28Z

+1 overall, just one minor style problem with boolean args to fix.

I think this is ready to go. You might also mention a couple of things in the PR description:

This moves parsing of DROP TABLE into Catalyst and adds parsed plans, like [SPARK-27108][SQL] Add parsed SQL plans for create, CTAS. #24029 did for create
Like [SPARK-27108][SQL] Add parsed SQL plans for create, CTAS. #24029, parsing tests for DROP TABLE have been moved to PlanResolutionSuite to validate existing behavior, and new tests for the catalyst parser have been added to Catalyst's DDLParserSuite

@cloud-fan, @dongjoon-hyun, could you review this DSv2 PR? I think it is ready to merge.

dongjoon-hyun · 2019-05-29T18:52:31Z

Thank you for pinging me, @rdblue . Yep. I'll take a look, too~

SparkQA · 2019-05-29T19:03:31Z

Test build #105920 has finished for PR 24686 at commit 780f7ff.

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

SparkQA · 2019-05-29T19:38:30Z

Test build #105921 has finished for PR 24686 at commit dd5b799.

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

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

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

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

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

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

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

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

SparkQA · 2019-05-30T06:13:04Z

Test build #105941 has finished for PR 24686 at commit d5b0d4a.

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

cloud-fan · 2019-05-30T16:56:26Z

thanks, merging to master!

jzhuge · 2019-05-30T17:33:34Z

Thanks @rdblue @dongjoon-hyun @cloud-fan !

## What changes were proposed in this pull request? Support DROP TABLE from V2 catalogs. Move DROP TABLE into catalyst. Move parsing tests for DROP TABLE/VIEW to PlanResolutionSuite to validate existing behavior. Add new tests fo catalyst parser suite. Separate DROP VIEW into different code path from DROP TABLE. Move DROP VIEW into catalyst as a new operator. Add a meaningful exception to indicate view is not currently supported in v2 catalog. ## How was this patch tested? New unit tests. Existing unit tests in catalyst and sql core. Closes apache#24686 from jzhuge/SPARK-27813-pr. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

jzhuge changed the title ~~Spark 27813 pr~~ [SPARK-27813][SQL] DataSourceV2: Add DropTable logical operation May 23, 2019

rdblue reviewed May 23, 2019

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/IdentifierImpl.java Outdated Show resolved Hide resolved

jzhuge closed this May 24, 2019

rdblue reviewed May 24, 2019

View reviewed changes

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

rdblue reviewed May 24, 2019

View reviewed changes

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

rdblue reviewed May 24, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalog/v2/TestTableCatalog.scala Outdated Show resolved Hide resolved

rdblue reviewed May 24, 2019

View reviewed changes

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

rdblue reviewed May 24, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala Outdated Show resolved Hide resolved

rdblue reviewed May 24, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalog/v2/LookupCatalog.scala Outdated Show resolved Hide resolved

[SPARK-26946][SQL][FOLLOWUP] Handle lookupCatalog function not defined

a79cb84

Treat it as catalog not found.

jzhuge reopened this May 26, 2019

jzhuge force-pushed the SPARK-27813-pr branch from de0ab34 to 4b2fd3c Compare May 26, 2019 22:41

[SPARK-27813][SQL] DataSourceV2: Add DropTable logical operation

db4a417

jzhuge force-pushed the SPARK-27813-pr branch from 4b2fd3c to db4a417 Compare May 26, 2019 22:44

jzhuge mentioned this pull request May 26, 2019

[SPARK-26946][SQL][FOLLOWUP] Require lookup function #24689

Closed

jzhuge mentioned this pull request May 27, 2019

[SPARK-27322][SQL] DataSourceV2: Select from multiple catalogs #24718

Closed

rdblue reviewed May 27, 2019

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/CatalogManager.java Outdated Show resolved Hide resolved

rdblue reviewed May 27, 2019

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/Catalogs.java Outdated Show resolved Hide resolved

rdblue reviewed May 27, 2019

View reviewed changes

...lyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/DropTableStatement.scala Outdated Show resolved Hide resolved

rdblue reviewed May 27, 2019

View reviewed changes

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

rdblue reviewed May 27, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala Outdated Show resolved Hide resolved

jzhuge added 4 commits May 29, 2019 08:28

Revert "[SPARK-26946][SQL][FOLLOWUP] Handle lookupCatalog function no…

424df75

…t defined" This reverts commit a79cb84

[SPARK-26946][SQL][FOLLOWUP] Require lookup function

7e11ca6

Review comments

780f7ff

Revert "[SPARK-26946][SQL][FOLLOWUP] Require lookup function"

dd5b799

This reverts commit 7e11ca6

rdblue reviewed May 29, 2019

View reviewed changes

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

rdblue reviewed May 29, 2019

View reviewed changes

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