address PR comments

Author: imback82

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

@@ -18,6 +18,7 @@
 package org.apache.spark.sql.execution.command
 
 import org.apache.spark.sql.{AnalysisException, QueryTest}
+import org.apache.spark.sql.connector.catalog.SupportsNamespaces
 
 /**
  * This base suite contains unified tests for the `ALTER NAMESPACE ... SET LOCATION` command that
@@ -36,6 +37,8 @@ import org.apache.spark.sql.{AnalysisException, QueryTest}
 trait AlterNamespaceSetLocationSuiteBase extends QueryTest with DDLCommandTestUtils {
   override val command = "ALTER NAMESPACE ... SET LOCATION"
 
+  protected def namespace: String
+
   protected def notFoundMsgPrefix: String
 
   test("Namespace does not exist") {
@@ -45,4 +48,27 @@ trait AlterNamespaceSetLocationSuiteBase extends QueryTest with DDLCommandTestUt
     }.getMessage
     assert(message.contains(s"$notFoundMsgPrefix '$ns' not found"))
   }
+
+  // Hive catalog does not support "ALTER NAMESPACE ... SET LOCATION", thus
+  // this is called from non-Hive v1 and v2 tests.
+  protected def runBasicTest(): Unit = {
+    val ns = s"$catalog.$namespace"
+    withNamespace(ns) {
+      sql(s"CREATE NAMESPACE IF NOT EXISTS $ns COMMENT " +
+        "'test namespace' LOCATION '/tmp/loc_test_1'")
+      sql(s"ALTER NAMESPACE $ns SET LOCATION '/tmp/loc_test_2'")
+      // v1 command stores the location as URI ("file:/tmp/loc_test_2") whereas
+      // v2 command stores the location as string ("/tmp/loc_test_2").
+      assert(getLocation(ns).contains("/tmp/loc_test_2"))
+    }
+  }
+
+  protected def getLocation(namespace: String): String = {
+    val locationRow = sql(s"DESCRIBE NAMESPACE EXTENDED $namespace")
+      .toDF("key", "value")
+      .where(s"key like '${SupportsNamespaces.PROP_LOCATION.capitalize}%'")
+      .collect()
+    assert(locationRow.length == 1)
+    locationRow(0).getString(1)
+  }
 }

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

@@ -28,7 +28,7 @@ import org.apache.spark.{SparkException, SparkFiles}
 import org.apache.spark.internal.config
 import org.apache.spark.sql.{AnalysisException, QueryTest, Row, SaveMode}
 import org.apache.spark.sql.catalyst.{FunctionIdentifier, QualifiedTableName, TableIdentifier}
-import org.apache.spark.sql.catalyst.analysis.{FunctionRegistry, NoSuchDatabaseException, NoSuchFunctionException, TableFunctionRegistry, TempTableAlreadyExistsException}
+import org.apache.spark.sql.catalyst.analysis.{FunctionRegistry, NoSuchFunctionException, TableFunctionRegistry, TempTableAlreadyExistsException}
 import org.apache.spark.sql.catalyst.catalog._
 import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
 import org.apache.spark.sql.connector.catalog.SupportsNamespaces.PROP_OWNER
@@ -776,17 +776,6 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
             Row("Comment", "") ::
             Row("Location", CatalogUtils.URIToString(location)) ::
             Row("Properties", "((a,a), (b,b), (c,c), (d,d))") :: Nil)
-
-        withTempDir { tmpDir =>
-          intercept[NoSuchDatabaseException] {
-            sql(s"ALTER DATABASE `db-not-exist` SET LOCATION '${tmpDir.toURI}'")
-          }
-
-          val e3 = intercept[IllegalArgumentException] {
-            sql(s"ALTER DATABASE $dbName SET LOCATION ''")
-          }
-          assert(e3.getMessage.contains("Can not create a Path from an empty string"))
-        }
       } finally {
         catalog.reset()
       }

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

@@ -17,8 +17,6 @@
 
 package org.apache.spark.sql.execution.command.v1
 
-import org.apache.hadoop.fs.Path
-
 import org.apache.spark.sql.execution.command
 
 /**
@@ -33,14 +31,15 @@ import org.apache.spark.sql.execution.command
  */
 trait AlterNamespaceSetLocationSuiteBase extends command.AlterNamespaceSetLocationSuiteBase
     with command.TestsV1AndV2Commands {
+  override def namespace: String = "db"
   override def notFoundMsgPrefix: String = "Database"
 
-  test ("Empty location string") {
-    val ns = "db1"
+  test("Empty location string") {
+    val ns = s"$catalog.$namespace"
     withNamespace(ns) {
-      sql(s"CREATE NAMESPACE $catalog.$ns")
+      sql(s"CREATE NAMESPACE $ns")
       val message = intercept[IllegalArgumentException] {
-        sql(s"ALTER DATABASE $catalog.$ns SET LOCATION ''")
+        sql(s"ALTER NAMESPACE $ns SET LOCATION ''")
       }.getMessage
       assert(message.contains("Can not create a Path from an empty string"))
     }
@@ -55,17 +54,7 @@ class AlterNamespaceSetLocationSuite extends AlterNamespaceSetLocationSuiteBase
     with CommandSuiteBase {
   override def commandVersion: String = super[AlterNamespaceSetLocationSuiteBase].commandVersion
 
-  test("basic v1 test") {
-    val ns = "db1"
-    withNamespace(ns) {
-      sql(s"CREATE NAMESPACE $catalog.$ns")
-      withTempDir { tmpDir =>
-        sql(s"ALTER NAMESPACE $catalog.$ns SET LOCATION '${tmpDir.toURI}'")
-        val sessionCatalog = spark.sessionState.catalog
-        val uriInCatalog = sessionCatalog.getDatabaseMetadata(ns).locationUri
-        assert("file" === uriInCatalog.getScheme)
-        assert(new Path(tmpDir.getPath).toUri.getPath === uriInCatalog.getPath)
-      }
-    }
+  test("basic test") {
+    runBasicTest()
   }
 }

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

@@ -17,33 +17,27 @@
 
 package org.apache.spark.sql.execution.command.v2
 
-import org.apache.spark.sql.Row
-import org.apache.spark.sql.connector.catalog.SupportsNamespaces
 import org.apache.spark.sql.execution.command
-import org.apache.spark.util.Utils
 
 /**
  * The class contains tests for the `ALTER NAMESPACE ... SET LOCATION` command to check V2 table
  * catalogs.
  */
 class AlterNamespaceSetLocationSuite extends command.AlterNamespaceSetLocationSuiteBase
     with CommandSuiteBase {
+  override def namespace: String = "ns1.ns2"
   override def notFoundMsgPrefix: String = "Namespace"
 
-  test("basic v2 test") {
-    val ns = "ns1.ns2"
-    withNamespace(s"$catalog.$ns") {
-      sql(s"CREATE NAMESPACE IF NOT EXISTS $catalog.$ns COMMENT " +
-        "'test namespace' LOCATION '/tmp/ns_test_1'")
-      sql(s"ALTER NAMESPACE $catalog.$ns SET LOCATION '/tmp/ns_test_2'")
-      val descriptionDf = sql(s"DESCRIBE NAMESPACE EXTENDED $catalog.$ns")
-      assert(descriptionDf.collect() === Seq(
-        Row("Namespace Name", "ns2"),
-        Row(SupportsNamespaces.PROP_COMMENT.capitalize, "test namespace"),
-        Row(SupportsNamespaces.PROP_LOCATION.capitalize, "/tmp/ns_test_2"),
-        Row(SupportsNamespaces.PROP_OWNER.capitalize, Utils.getCurrentUserName()),
-        Row("Properties", ""))
-      )
+  test("basic test") {
+    runBasicTest()
+  }
+
+  test("Empty location string is allowed") {
+    val ns = s"$catalog.$namespace"
+    withNamespace(ns) {
+      sql(s"CREATE NAMESPACE $ns")
+      sql(s"ALTER NAMESPACE $ns SET LOCATION ''")
+      assert(getLocation(ns) === "")
     }
   }
 }

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

@@ -29,7 +29,7 @@ class AlterNamespaceSetLocationSuite extends v1.AlterNamespaceSetLocationSuiteBa
   override def commandVersion: String = super[AlterNamespaceSetLocationSuiteBase].commandVersion
 
   test("Hive catalog not supported") {
-    val ns = "db1"
+    val ns = s"$catalog.$namespace"
     withNamespace(ns) {
       sql(s"CREATE NAMESPACE $ns")
       val e = intercept[AnalysisException] {