[SPARK-37804][SQL][TEST] Unify v1 and v2 CREATE NAMESPACE tests #35093

imback82 · 2022-01-04T06:42:50Z

What changes were proposed in this pull request?

Move CREATE NAMESPACE parsing tests to CreateNamespaceParserSuite.
Put common CREATE NAMESPACE tests into one trait org.apache.spark.sql.execution.command.CreateNamespaceSuiteBase, and put datasource specific tests to the v1.CreateNamespaceSuite and v2.CreateNamespaceSuite.

The changes follow the approach of #30287.

Why are the changes needed?

The unification will allow to run common CREATE NAMESPACE tests for both DSv1/Hive DSv1 and DSv2
We can detect missing features and differences between DSv1 and DSv2 implementations.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing unit tests and new tests.

imback82 · 2022-01-04T06:43:40Z

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

-    }
-  }
-
-  test("SPARK-37456: Location in CreateNamespace should be qualified") {


This should be covered in test("namespace with location") for all catalogs.

imback82 · 2022-01-04T06:44:46Z

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

+      sql(s"CREATE NAMESPACE IF NOT EXISTS $ns")
+
+      // The namespace already exists, so this should fail.
+      // TODO: non-Hive catalogs throw NamespaceAlreadyExistsException.


This is a new TODO similar to // TODO: HiveExternalCatalog throws DatabaseAlreadyExistsException. Basically, hive catalog throws a different exception (and a slightly different message, requiring alreadyExistErrorMessage).

imback82 · 2022-01-04T06:45:20Z

cc @cloud-fan @MaxGekk. Happy New Year!

imback82 · 2022-01-04T06:46:23Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateNamespaceSuite.scala

+ */
+class CreateNamespaceSuite extends v1.CreateNamespaceSuiteBase with CommandSuiteBase {
+  override def commandVersion: String = super[CreateNamespaceSuiteBase].commandVersion
+  override def alreadyExistErrorMessage: String = s"$notFoundMsgPrefix $namespace already exists"


For Hive, there is no ' ' around the namespace.

is this exception thrown by hive? or by Spark?

This is thrown by Hive: org.apache.hadoop.hive.metastore.api.AlreadyExistsException: Database db already exists and wrapped to AnalysisException here:

spark/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

Lines 111 to 112 in 9c6f3fb

throw new AnalysisException(

e.getClass.getCanonicalName + ": " + e.getMessage, cause = Some(e))

imback82 · 2022-01-04T06:46:54Z

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

+  override def namespace: String = "db"
+  override def notFoundMsgPrefix: String = "Database"
+
+  test("Create namespace using default warehouse path") {


This is the only difference b/w v1 and v2 catalog.

cloud-fan · 2022-01-04T08:51:00Z

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

+trait CreateNamespaceSuiteBase extends QueryTest with DDLCommandTestUtils {
+  import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
+
+  override val command = "Create NAMESPACE"


Suggested change

override val command = "Create NAMESPACE"

override val command = "CREATE NAMESPACE"

cloud-fan · 2022-01-04T08:52:33Z

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

+        assert(!path.startsWith("file:/"))
+
+        val e = intercept[IllegalArgumentException] {
+          sql(s"CREATE NAMESPACE $ns Location ''")


Suggested change

sql(s"CREATE NAMESPACE $ns Location ''")

sql(s"CREATE NAMESPACE $ns LOCATION ''")

cloud-fan · 2022-01-04T08:52:42Z

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

+        assert(e.getMessage.contains("Can not create a Path from an empty string"))
+
+        val uri = new Path(path).toUri
+        sql(s"CREATE NAMESPACE $ns Location '$uri'")


Suggested change

sql(s"CREATE NAMESPACE $ns Location '$uri'")

sql(s"CREATE NAMESPACE $ns LOCATION '$uri'")

cloud-fan · 2022-01-04T08:55:03Z

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

+    }
+  }
+
+  test("test handling of 'IF NOT EXIST'") {


shall we merge this test case to the one above? It's just adding sql(s"CREATE NAMESPACE IF NOT EXISTS $ns") to the end of the above test case

cloud-fan · 2022-01-05T08:12:43Z

thanks, merging to master!

### What changes were proposed in this pull request? This PR proposes to use V2 commands as default as outlined in [SPARK-36588](https://issues.apache.org/jira/browse/SPARK-36588), and this PR migrates `CREATE NAMESPACE` to use v2 command by default. Note that the work to tests covering both v1/v2 were done in #35093. ### Why are the changes needed? It's been a while since we introduced the v2 commands, and it seems reasonable to use v2 commands by default even for the session catalog, with a legacy config to fall back to the v1 commands. ### Does this PR introduce _any_ user-facing change? The error message will be slightly different if namespace already exists when v2 command is run against v1 catalog: v1 command: `Database 'db' already exists"` vs. v2 command: `Namespace 'db' already exists"` Also, the error message will be slightly different if namespace already exists when v1 command is run against Hive catalog: Before: `Database db already exists"` vs. After: `Database 'db' already exists"` ### How was this patch tested? Existing *CreateNamespaceSuite tests cover this PR's change. Closes #35113 from imback82/migrate_create_namespace. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This PR proposes to use V2 commands as default as outlined in [SPARK-36588](https://issues.apache.org/jira/browse/SPARK-36588), and this PR migrates `CREATE NAMESPACE` to use v2 command by default. Note that the work to tests covering both v1/v2 were done in apache#35093. ### Why are the changes needed? It's been a while since we introduced the v2 commands, and it seems reasonable to use v2 commands by default even for the session catalog, with a legacy config to fall back to the v1 commands. ### Does this PR introduce _any_ user-facing change? The error message will be slightly different if namespace already exists when v2 command is run against v1 catalog: v1 command: `Database 'db' already exists"` vs. v2 command: `Namespace 'db' already exists"` Also, the error message will be slightly different if namespace already exists when v1 command is run against Hive catalog: Before: `Database db already exists"` vs. After: `Database 'db' already exists"` ### How was this patch tested? Existing *CreateNamespaceSuite tests cover this PR's change. Closes apache#35113 from imback82/migrate_create_namespace. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

imback82 added 3 commits January 3, 2022 10:21

initial commit

1513b88

added new tests

34ac845

tests

34dfed0

github-actions bot added the SQL label Jan 4, 2022

imback82 commented Jan 4, 2022

View reviewed changes

fix typo

864a16c

cloud-fan reviewed Jan 4, 2022

View reviewed changes

address PR comments

499a3f9

cloud-fan approved these changes Jan 5, 2022

View reviewed changes

cloud-fan closed this in 33bdfba Jan 5, 2022

imback82 mentioned this pull request Jan 6, 2022

[SPARK-37636][SQL] Migrate CREATE NAMESPACE to use V2 command by default #35113

Closed

imback82 deleted the v2_create_namespace branch January 7, 2022 06:33

	throw new AnalysisException(
	e.getClass.getCanonicalName + ": " + e.getMessage, cause = Some(e))

	override val command = "Create NAMESPACE"
	override val command = "CREATE NAMESPACE"

	sql(s"CREATE NAMESPACE $ns Location ''")
	sql(s"CREATE NAMESPACE $ns LOCATION ''")

	sql(s"CREATE NAMESPACE $ns Location '$uri'")
	sql(s"CREATE NAMESPACE $ns LOCATION '$uri'")

[SPARK-37804][SQL][TEST] Unify v1 and v2 CREATE NAMESPACE tests #35093

[SPARK-37804][SQL][TEST] Unify v1 and v2 CREATE NAMESPACE tests #35093

Uh oh!

Conversation

imback82 commented Jan 4, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imback82 commented Jan 4, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants