[SPARK-19235] [SQL] [TESTS] Enable Test Cases in DDLSuite with Hive Metastore #16592

gatorsmile · 2017-01-16T05:25:06Z

What changes were proposed in this pull request?

So far, the test cases in DDLSuites only verify the behaviors of InMemoryCatalog. That means, they do not cover the scenarios using HiveExternalCatalog. Thus, we need to improve the existing test suite to run these cases using Hive metastore.

When porting these test cases, a bug of SET LOCATION is found. path is not set when the location is changed.

After this PR, a few changes are made, as summarized below,

DDLSuite becomes an abstract class. Both InMemoryCatalogedDDLSuite and HiveCatalogedDDLSuite extend it. InMemoryCatalogedDDLSuite is using InMemoryCatalog. HiveCatalogedDDLSuite is using HiveExternalCatalog.
InMemoryCatalogedDDLSuite contains all the existing test cases in DDLSuite.
HiveCatalogedDDLSuite contains a subset of DDLSuite. The following test cases are excluded:

The following test cases only make sense for InMemoryCatalog:

  test("desc table for parquet data source table using in-memory catalog")
  test("create a managed Hive source table") {
  test("create an external Hive source table")
  test("Create Hive Table As Select")

The following test cases are unable to be ported because we are unable to alter table provider when using Hive metastore. In the future PRs we need to improve the test cases so that altering table provider is not needed:

  test("alter table: set location (datasource table)")
  test("alter table: set properties (datasource table)")
  test("alter table: unset properties (datasource table)")
  test("alter table: set serde (datasource table)")
  test("alter table: set serde partition (datasource table)")
  test("alter table: change column (datasource table)")
  test("alter table: add partition (datasource table)")
  test("alter table: drop partition (datasource table)")
  test("alter table: rename partition (datasource table)")
  test("drop table - data source table")

TODO : in the future PRs, we need to remove HiveDDLSuite and move the test cases to either DDLSuite, InMemoryCatalogedDDLSuite or HiveCatalogedDDLSuite.

How was this patch tested?

N/A

gatorsmile · 2017-01-16T05:26:57Z

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

+      }.getMessage
+      assert(e.contains("Hive support is required to use CREATE Hive TABLE AS SELECT"))
+    }
+  }


The above four cases are copied from the existing ones in DDLSuites. These test cases only makes sense to InMemoryCatalog.

gatorsmile · 2017-01-16T05:28:29Z

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

+
+  test("drop table - data source table") {
+    testDropTable(isDatasourceTable = true)
+  }


The above 10 test cases are currently running with InMemoryCatalog only. The reason is HiveExternalCatalog does not allow users to change the table provider from hive to the others. In the future PRs, we can fix it.

gatorsmile · 2017-01-16T05:33:35Z

cc @cloud-fan I think we really need to do this ASAP for improving the test case coverage in DDL commands, when I do the PR: #16587

SparkQA · 2017-01-16T07:55:27Z

Test build #71420 has finished for PR 16592 at commit 0133463.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InMemoryCatalogedDDLSuite extends DDLSuite with SharedSQLContext with BeforeAndAfterEach
abstract class DDLSuite extends QueryTest with SQLTestUtils
class HiveCatalogedDDLSuite extends DDLSuite with TestHiveSingleton with BeforeAndAfterEach

cloud-fan · 2017-01-16T08:45:28Z

can we wait for #16597?

gatorsmile · 2017-01-16T17:34:41Z

Sure, no problem.

gatorsmile · 2017-01-17T19:32:30Z

retest this please

SparkQA · 2017-01-17T21:11:31Z

Test build #71526 has finished for PR 16592 at commit 0133463.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InMemoryCatalogedDDLSuite extends DDLSuite with SharedSQLContext with BeforeAndAfterEach
abstract class DDLSuite extends QueryTest with SQLTestUtils
class HiveCatalogedDDLSuite extends DDLSuite with TestHiveSingleton with BeforeAndAfterEach

gatorsmile · 2017-01-19T08:46:42Z

retest this please

SparkQA · 2017-01-19T11:10:25Z

Test build #71651 has finished for PR 16592 at commit 0133463.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class InMemoryCatalogedDDLSuite extends DDLSuite with SharedSQLContext with BeforeAndAfterEach
abstract class DDLSuite extends QueryTest with SQLTestUtils
class HiveCatalogedDDLSuite extends DDLSuite with TestHiveSingleton with BeforeAndAfterEach

gatorsmile · 2017-01-20T02:43:58Z

ping @cloud-fan

cloud-fan · 2017-01-20T08:30:00Z

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

+
+  private val escapedIdentifier = "`(.+)`".r
+
+  def normalizeCatalogTable(table: CatalogTable): CatalogTable = {


I think we only need this in HiveCatalogedDDLSuite?

Yes. Let me move it to HiveCatalogedDDLSuite

cloud-fan · 2017-01-20T08:35:41Z

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

-        intercept[DatabaseAlreadyExistsException] {
-          sql(s"CREATE DATABASE $dbName")
+        if (isUsingHiveMetastore) {
+          val e = intercept[AnalysisException] {


we can write intercept[AnalysisException] and leave a TODO saying that HiveExternalCatalog should throw DatabaseAlreadyExistsException

cloud-fan · 2017-01-20T08:37:48Z

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

+      val dbPath = new Path(spark.sessionState.conf.warehousePath)
+      s"${dbPath.toUri.getPath.stripSuffix("/")}/$dbName.db"
+    } else {
+      s"spark-warehouse/$dbName.db"


why hardcode it?

: ) copied from the original place. Let me clean it. Thanks!

SparkQA · 2017-01-20T23:20:20Z

Test build #71740 has finished for PR 16592 at commit b905191.

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

xwu0226 · 2017-02-24T20:17:15Z

@gatorsmile is this PR going to be merged soon, since the ALTER ADD column PR #16626 also depends on this to create test cases for InMemoryCatalog. Thanks!

gatorsmile · 2017-02-25T06:29:05Z

Let me continue the work now

SparkQA · 2017-03-06T07:12:41Z

Test build #73975 has started for PR 16592 at commit b186dcd.

gatorsmile · 2017-03-06T20:29:49Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

    }
  }

-  Seq("a b", "a:b", "a%b").foreach { specialChars =>


This test case is duplicate from the one in HiveCatalogedDDLSuite. Removed.

gatorsmile · 2017-03-06T20:29:57Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

  }
-
-  Seq("a b", "a:b", "a%b").foreach { specialChars =>
-    test(s"location uri contains $specialChars for database") {


This test case is duplicate from the one in HiveCatalogedDDLSuite. Removed.

SparkQA · 2017-03-06T22:35:21Z

Test build #74037 has finished for PR 16592 at commit d19d725.

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

gatorsmile · 2017-03-06T22:45:18Z

cc @cloud-fan

SparkQA · 2017-03-07T19:23:38Z

Test build #74116 has finished for PR 16592 at commit ec914dc.

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

cloud-fan · 2017-03-07T21:32:10Z

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

-      val expectedLocation = makeQualifiedPath(s"spark-warehouse/$dbName.db")
+      val dbPath = new Path(spark.sessionState.conf.warehousePath)
+      val expectedDBLocation = s"file:${dbPath.toUri.getPath.stripSuffix("/")}/$dbName.db"
+      val expectedDBUri = CatalogUtils.stringToURI(expectedDBLocation)


how about

val expectedDBPath = new Path(spark.sessionState.conf.warehousePath, s"$dbName.db").toURI

cloud-fan · 2017-03-07T21:32:46Z

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

+      name: TableIdentifier): CatalogTable = {
+    val storage =
+      CatalogStorageFormat(
+        locationUri = Some(catalog.defaultTablePath(name)),


nit: CatalogStorageFormat.copy(locationUri = xxx)

cloud-fan · 2017-03-07T21:50:46Z

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

+        val dbPath = new Path(spark.sessionState.conf.warehousePath)
+        val expectedDBLocation =
+          s"file:${dbPath.toUri.getPath.stripSuffix("/")}/$dbNameWithoutBackTicks.db"
+        val expectedDBUri = CatalogUtils.stringToURI(expectedDBLocation)


cloud-fan · 2017-03-07T21:51:10Z

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

        val dbNameWithoutBackTicks = cleanIdentifier(dbName)
-        val location = makeQualifiedPath(s"spark-warehouse/$dbNameWithoutBackTicks.db")
+        val dbPath = new Path(spark.sessionState.conf.warehousePath)
+        val expectedDBLocation =


cloud-fan · 2017-03-07T21:51:33Z

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

        sql(s"DROP DATABASE $dbName")
      }.getMessage
-      assert(message.contains(s"Database '$dbNameWithoutBackTicks' not found"))
+      if (isUsingHiveMetastore) {


add a TODO to unify the exception?

cloud-fan · 2017-03-07T21:51:43Z

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

      sql(s"DROP DATABASE $dbName RESTRICT")
    }.getMessage
-    assert(message.contains(s"Database '$dbName' is not empty. One or more tables exist"))
+    if (isUsingHiveMetastore) {


add a TODO to unify the exception?

cloud-fan · 2017-03-07T21:52:45Z

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

-      assert(catalog.getPartition(tableIdent, part1).parameters("numFiles") == "1")
-      assert(catalog.getPartition(tableIdent, part2).parameters("numFiles") == "2")
+      if (!isUsingHiveMetastore) {
+        assert(catalog.getPartition(tableIdent, part1).parameters("numFiles") == "1")


why only test it for InMemoryExternalCatalog?

After ALTER TABLE, the statistics info of the first partition is removed by Hive megastore. : (

Let me write a comment in the code.

cloud-fan · 2017-03-07T21:53:36Z

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

-  }
-
-  private def testDropTable(isDatasourceTable: Boolean): Unit = {
+  protected def testDropTable(isDatasourceTable: Boolean): Unit = {


if it's only used in InMemoryCatalogedDDLSuite,we should move this method there

This is not only for InMemoryCatalogedDDLSuite . We have another call here.

cloud-fan · 2017-03-07T22:06:23Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

+      "maxFileSize",
+      "minFileSize",
+      // EXTERNAL is not non-deterministic, but it is filtered out for external tables.
+      "EXTERNAL"


this is gone now.

SparkQA · 2017-03-08T02:51:58Z

Test build #74160 has finished for PR 16592 at commit 64a8f5a.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-08T17:38:34Z

retest this please

SparkQA · 2017-03-08T19:47:35Z

Test build #74213 has finished for PR 16592 at commit 64a8f5a.

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

cloud-fan · 2017-03-08T21:26:33Z

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

+    val partitionLocation = if (isUsingHiveMetastore) {
+      val tableLocation = catalog.getTableMetadata(tableIdent).storage.locationUri
+      assert(tableLocation.isDefined)
+      makeQualifiedPath(tableLocation.get + "/paris")


we should avoid hardcoding file seperator

SparkQA · 2017-03-09T00:54:37Z

Test build #74223 has finished for PR 16592 at commit 9d4211c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public static class LongWrapper
public static class IntWrapper
case class CostBasedJoinReorder(conf: CatalystConf) extends Rule[LogicalPlan] with PredicateHelper
case class JoinPlan(itemIds: Set[Int], plan: LogicalPlan, joinConds: Set[Expression], cost: Cost)
case class Cost(rows: BigInt, size: BigInt)
abstract class RepartitionOperation extends UnaryNode
case class FlatMapGroupsWithState(
trait WatermarkSupport extends UnaryExecNode
case class FlatMapGroupsWithStateExec(

windpiger · 2017-03-09T01:17:34Z

great~ waiting this to merge and fix my confilicts~

gatorsmile · 2017-03-09T01:32:52Z

ping @cloud-fan

cloud-fan · 2017-03-09T07:02:24Z

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

  }

+  private def getDBPath(dbName: String): URI = {
+    val warehousePath = s"file:${spark.sessionState.conf.warehousePath.stripPrefix("file:")}"


we can call makeQualifiedPath here. We can fix it in follow up.

cloud-fan · 2017-03-09T07:11:25Z

sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala

    fs.makeQualified(hadoopPath).toUri
  }
+
+  def makeQualifiedPath(path: Path): URI = {


actually we don't need this, we already have def makeQualifiedPath(path: String): URI. let's remove it in follow up

I have fixed the above two issues in #17095

cloud-fan · 2017-03-09T07:12:38Z

great work! merging to master!

xubo245 · 2018-01-11T08:29:14Z

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

-      }
+      // TODO(gatorsmile): fix the bug in alter table set location.
+      // if (isUsingHiveMetastore) {
+      //  assert(storageFormat.properties.get("path") === expected)


Do we need to fix this bug and satify this test case?
When porting these test cases, a bug of SET LOCATION is found. path is not set when the location is changed.

Yeah. We need to fix it. Thanks for working on it.

fix

0133463

gatorsmile commented Jan 16, 2017

View reviewed changes

gatorsmile mentioned this pull request Jan 17, 2017

[SPARK-19240][SQL][TEST] add test for setting location for managed table #16597

Closed

Merge remote-tracking branch 'upstream/master' into refactorDDLSuite

1946f93

cloud-fan reviewed Jan 20, 2017

View reviewed changes

gatorsmile added 2 commits January 20, 2017 12:39

address comments.

589e995

Merge remote-tracking branch 'upstream/master' into refactorDDLSuite

b905191

gatorsmile mentioned this pull request Jan 22, 2017

[SPARK-19329][SQL]Reading from or writing to a datasource table with a non pre-existing location should succeed #16672

Closed

gatorsmile mentioned this pull request Feb 20, 2017

[SPARK-19261][SQL] Alter add columns for Hive serde and some datasource tables #16626

Closed

Merge remote-tracking branch 'upstream/master' into refactorDDLSuite

b186dcd

gatorsmile mentioned this pull request Mar 6, 2017

[SPARK-19257][SQL]location for table/partition/database should be java.net.URI #17149

Closed

Merge remote-tracking branch 'upstream/master' into refactorDDLSuite

d19d725

gatorsmile commented Mar 6, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into refactorDDLSuite

ec914dc

cloud-fan reviewed Mar 7, 2017

View reviewed changes

gatorsmile added 2 commits March 7, 2017 16:59

address comments.

5fa51b5

address comments.

64a8f5a

cloud-fan reviewed Mar 8, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into refactorDDLSuite

9d4211c

cloud-fan reviewed Mar 9, 2017

View reviewed changes

asfgit closed this in 09829be Mar 9, 2017

xubo245 reviewed Jan 11, 2018

View reviewed changes


		private val escapedIdentifier = "`(.+)`".r

		def normalizeCatalogTable(table: CatalogTable): CatalogTable = {

[SPARK-19235] [SQL] [TESTS] Enable Test Cases in DDLSuite with Hive Metastore #16592

[SPARK-19235] [SQL] [TESTS] Enable Test Cases in DDLSuite with Hive Metastore #16592

Uh oh!

Conversation

gatorsmile commented Jan 16, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jan 16, 2017

Uh oh!

SparkQA commented Jan 16, 2017

Uh oh!

cloud-fan commented Jan 16, 2017

Uh oh!

gatorsmile commented Jan 16, 2017

Uh oh!

gatorsmile commented Jan 17, 2017

Uh oh!

SparkQA commented Jan 17, 2017

Uh oh!

gatorsmile commented Jan 19, 2017

Uh oh!

SparkQA commented Jan 19, 2017

Uh oh!

gatorsmile commented Jan 20, 2017

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!

SparkQA commented Jan 20, 2017

Uh oh!

xwu0226 commented Feb 24, 2017

Uh oh!

gatorsmile commented Feb 25, 2017

Uh oh!

SparkQA commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 6, 2017

Uh oh!

gatorsmile commented Mar 6, 2017

Uh oh!

SparkQA commented Mar 7, 2017

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

gatorsmile commented Jan 16, 2017 •

edited

Loading

windpiger commented Mar 9, 2017 •

edited

Loading

windpiger Mar 9, 2017 •

edited

Loading