[SPARK-37478][SQL][TESTS] Unify v1 and v2 DROP NAMESPACE tests #34819

dchvn · 2021-12-06T09:24:14Z

What changes were proposed in this pull request?

Move the DROP NAMESPACE parsing tests from DDLParserSuite to DropNamespaceParserSuite
Define the class command.DropNamespaceSuiteBase that is parent of v1.DropNamespaceSuiteBase and v2.DropNamespaceSuite.
Define the class v1.DropNamespaceSuiteBase that is parent of v1.DropNamespaceSuite and hive.execution.command.DropNamespaceSuite.
Place the v1 tests for DROP NAMESPACE from DDLSuite and v2 tests from DataSourceV2SQLSuite to the common trait DropNamespaceSuiteBase.
Move TODO test from DescribeNamespaceSuiteBase to v1.DropNamespaceSuiteBase and v2.DropNamespaceSuite

The changes follow the approach of #30287

Why are the changes needed?

The unification will allow to run common DROP 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?

v1/v2 and Hive v1 DropNamespaceSuite:

$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *DropNamespaceSuite"

dchvn · 2021-12-06T09:36:07Z

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

+  test("DropNamespace: Namespace does not exist") {
+    // Namespace $catalog.unknown does not exist.
+    val message = intercept[AnalysisException] {
+      sql(s"DROP DATABASE $catalog.unknown")
+    }.getMessage
+    assert(message.contains(s"Database 'unknown' not found"))


This test in V1 throws NoSuchDatabaseException that is different from NoSuchNamespaceException in V2. So it is separated into V1 and V2 Suite instead of placed in SuiteBase.

do these 2 exceptions both extends AnalysisException? We can check that instead.

dchvn · 2021-12-06T09:36:44Z

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

+  test("DropNamespace: Namespace does not exist") {
+    // Namespace $catalog.unknown does not exist.
+    val message = intercept[AnalysisException] {
+      sql(s"DROP DATABASE $catalog.unknown")
+    }.getMessage
+    assert(message.contains(s"Namespace 'unknown' not found"))


This test in V2 throws NoSuchNamespaceException that is different from NoSuchDatabaseException in V1. So it is separated into V1 and V2 Suite instead of placed in SuiteBase.

dchvn · 2021-12-06T09:43:40Z

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

+    def assertDropFails(): Unit = {
+      val e = intercept[AnalysisException] {
+        sql(s"DROP NAMESPACE $catalog.ns")
+      }
+      assert(e.getMessage.contains("Database ns is not empty. One or more tables exist"))


The exception and message of test drop non-empty namespace with a non-cascading mode that throw in V1 DropNamespaceSuite are different from V2 DropNamespaceSuite.

Can we unify it? We don't need to block this PR and can do this later.

dchvn · 2021-12-06T09:43:54Z

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

+    def assertDropFails(): Unit = {
+      val e = intercept[SparkException] {
+        sql(s"DROP NAMESPACE $catalog.ns")
+      }
+      assert(e.getMessage.contains("Cannot drop a non-empty namespace: ns"))


The exception and message of test drop non-empty namespace with a non-cascading mode that throw in V1 DropNamespaceSuite are different from V2 DropNamespaceSuite.

We can follow DescribeNamespaceSuiteBase and add a method to abstract out the differences in the error message.

dchvn · 2021-12-06T09:58:12Z

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

+trait DropNamespaceSuiteBase extends command.DropNamespaceSuiteBase {
+  override protected def builtinTopNamespaces: Seq[String] = Seq("default")
+
+  test("DropNamespace: drop default namespace") {


default namespace does not exist in V2, test for V1 and Hive only.

dchvn · 2021-12-06T10:02:00Z

@MaxGekk, @cloud-fan, @imback82 Could you take a look? Many thanks!

SparkQA · 2021-12-06T10:37:34Z

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

SparkQA · 2021-12-06T11:19:55Z

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

cloud-fan · 2021-12-06T12:26:07Z

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

+  protected def builtinTopNamespaces: Seq[String] = Seq.empty
+  protected def isCasePreserving: Boolean = true
+
+  protected def checkNamespace(sqlText: String, expected: Seq[String]) = {


since we don't need the sqlText parameter, as it's always SHOW NAMESPACES IN $catalog

SparkQA · 2021-12-06T14:36:36Z

Test build #145948 has finished for PR 34819 at commit c60e31b.

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

dchvn · 2021-12-07T03:04:31Z

ping @cloud-fan, updated as your reviews. Could you take a look? Thanks!

SparkQA · 2021-12-07T03:48:29Z

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

SparkQA · 2021-12-07T04:31:36Z

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

cloud-fan · 2021-12-07T04:55:58Z

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

+
+  protected def builtinTopNamespaces: Seq[String] = Seq.empty
+  protected def isCasePreserving: Boolean = true
+  protected def assertDropFails


Suggested change

protected def assertDropFails

protected def assertDropFails(): Unit

cloud-fan · 2021-12-07T04:56:50Z

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

+  test("DropNamespace: Namespace does not exist") {
+    // Namespace $catalog.unknown does not exist.
+    val message = intercept[AnalysisException] {
+      sql(s"DROP DATABASE $catalog.unknown")


Can we consistently use DROP NAMESPACE in the tests?

cloud-fan · 2021-12-07T04:57:14Z

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

+    checkNamespace(Seq("ns") ++ builtinTopNamespaces)
+
+    // $catalog.ns.table is present, thus $catalog.ns cannot be dropped.
+    assertDropFails


Suggested change

assertDropFails

assertDropFails()

cloud-fan · 2021-12-07T04:58:34Z

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

+    checkNamespace(builtinTopNamespaces)
+  }
+
+  test("DropNamespace: drop namespace with case sensitivity") {


nit: this is the drop namespace test suite, do we really need to put the DropNamespace: prefix in all the test names here?

yeah, will remove it. Thanks!

cloud-fan · 2021-12-07T05:00:21Z

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

+ */
+class DropNamespaceSuite extends command.DropNamespaceSuiteBase with CommandSuiteBase {
+  override protected def assertDropFails(): Unit = {
+    val e = intercept[SparkException] {


Can we leave a TODO to unify the error? I think both v1 and v2 commands should throw AnalysisException, and the error message should only have one difference between v1 and v2: Cannot drop a non-empty namespace: ... vs Cannot drop a non-empty database: ...

If you think the change is small, you can also include it in this PR

I will create a follow-up PR for this, it's ok? @cloud-fan

Can we leave a TODO to unify the error? I think both v1 and v2 commands should throw AnalysisException, and the error message should only have one difference between v1 and v2: Cannot drop a non-empty namespace: ... vs Cannot drop a non-empty database: ...

The error message of Hive External Catalog test is Database ns is not empty. One or more tables exist., which is identical to the error message of v1. So we should keep this and remove TODO, or we should unify the error of v1, v2 test and then seperate the hive's one. Thanks. @cloud-fan @huaxingao

can we change the v1 error message to match v2?

yes, we can. After that, the test in Hive External Catalog would fail since it extends v1.DropNamespaceSuiteBase. So I think we should unify the error of v1, v2 test, and then create another one in Hive external catalog test that contains error message Database ns is not empty. One or more tables exist.. WDYT, @cloud-fan ?

can we unify the error message everywhere? We can try-catch the error from hive catalog and re-throw it with a different error message?

yeah, I think we can. I'll try. Thank you, @cloud-fan. 😃

cloud-fan

LGTM except for a few comments

SparkQA · 2021-12-07T08:15:28Z

Test build #145971 has finished for PR 34819 at commit 3970d4d.

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

SparkQA · 2021-12-07T08:32:47Z

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

SparkQA · 2021-12-07T09:32:39Z

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

SparkQA · 2021-12-07T12:41:55Z

Test build #145980 has finished for PR 34819 at commit 980e2be.

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

cloud-fan · 2021-12-07T12:49:56Z

thanks, merging to master!

dchvn · 2021-12-08T00:48:46Z

Thank you!

### What changes were proposed in this pull request? According to [#cmt](#34819 (comment)), unify the error of v1, v2 and hive external catalog in DROP NAMESPACE tests. ### Why are the changes needed? Currently, v1 and hive external catalog command throw `AnalysisException`, while v2 command throws `SparkException`. The error messages of v1 and hive catalog are also completely different from v2. So this PR is for unifying those errors to `AnalysisException`. The error message will have one difference between v1/hive and v2: `Cannot drop a non-empty database: ...` vs `Cannot drop a non-empty namespace: ...` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? v1/v2 and Hive v1 DropNamespaceSuite: ```$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *DropNamespaceSuite"``` Closes #35007 from dchvn/unify_dropnamespace_error. Authored-by: dch nguyen <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

dchvn added 2 commits December 6, 2021 13:45

[SPARK-37478] Unify v1 and v2 DROP NAMESPACE tests

275df3d

add new line to end of file

c60e31b

github-actions bot added the SQL label Dec 6, 2021

dchvn commented Dec 6, 2021

View reviewed changes

dchvn changed the title ~~[SPARK-33849][SQL][TESTS] Unify v1 and v2 DROP NAMESPACE tests~~ [SPARK-37478][SQL][TESTS] Unify v1 and v2 DROP NAMESPACE tests Dec 6, 2021

cloud-fan reviewed Dec 6, 2021

View reviewed changes

unify test

3970d4d

cloud-fan reviewed Dec 7, 2021

View reviewed changes

cloud-fan approved these changes Dec 7, 2021

View reviewed changes

resolve review

980e2be

cloud-fan approved these changes Dec 7, 2021

View reviewed changes

cloud-fan closed this in f6e2e65 Dec 7, 2021

dchvn mentioned this pull request Dec 24, 2021

[SPARK-37478][SQL][TESTS][FOLLOWUP] Unify v1 and v2 DROP NAMESPACE error #35007

Closed

	protected def assertDropFails
	protected def assertDropFails(): Unit

[SPARK-37478][SQL][TESTS] Unify v1 and v2 DROP NAMESPACE tests #34819

[SPARK-37478][SQL][TESTS] Unify v1 and v2 DROP NAMESPACE tests #34819

Uh oh!

Conversation

dchvn commented Dec 6, 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!

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!

dchvn commented Dec 6, 2021

Uh oh!

SparkQA commented Dec 6, 2021

Uh oh!

SparkQA commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 6, 2021

Uh oh!

dchvn commented Dec 7, 2021

Uh oh!

SparkQA commented Dec 7, 2021

Uh oh!

SparkQA commented Dec 7, 2021

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 7, 2021

Uh oh!

SparkQA commented Dec 7, 2021

Uh oh!

SparkQA commented Dec 7, 2021

Uh oh!

SparkQA commented Dec 7, 2021

Uh oh!

cloud-fan commented Dec 7, 2021