[SPARK-19331][SQL][TESTS] Improve the test coverage of SQLViewSuite #16674

jiangxb1987 · 2017-01-22T12:13:08Z

What changes were proposed in this pull request?

Move SQLViewSuite from sql/hive to sql/core, so we can test the view supports without hive metastore. Also moved the test cases that specified to hive to HiveSQLViewSuite.

Improve the test coverage of SQLViewSuite, cover the following cases:

view resolution(possibly a referenced table/view have changed after the view creation);
handle a view with user specified column names;
improve the test cases for a nested view.

Also added a test case for cyclic view reference, which is a known issue that is not fixed yet.

How was this patch tested?

N/A

SparkQA · 2017-01-22T14:00:33Z

Test build #71806 has finished for PR 16674 at commit 60d927d.

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

gatorsmile · 2017-01-22T19:27:17Z

I think we are having a big gap for testing view supports without using Hive metastore. So far, we only have a single test case you added. My proposal here is to move the whole test suite to the SQL module from the Hive module. Then, create another test suite in Hive that extends the suite in SQL.

gatorsmile · 2017-01-22T19:29:42Z

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

What is the reason?

How about creating a new test case? First, create a view using a regular SQL statement, and then fetch the metadata and verify the fetched metadata is still matching what we expect?

gatorsmile · 2017-01-22T19:33:20Z

I saw the PR deleted a few test cases. Could you post the reasons you did?

jiangxb1987 · 2017-01-23T01:45:52Z

@gatorsmile The only test cast that I removed is test("correctly resolve a view with CTE"), which is duplicate with the existing test("CTE within view").

For testing without using Hive metastore, I'll update the test suites follow your suggestion, maybe later in a few days, since you know, the Chinese New Year is coming :)

gatorsmile · 2017-01-26T06:15:56Z

Happy New Year! : )

I double checked all the test case change. The changes makes sense to me. Could you resolve the conflicts?

BTW, a general suggestion. Could you please add the comments when you move the codes in the PR? It can help others review the codes.

SparkQA · 2017-02-08T06:58:34Z

Test build #72567 has started for PR 16674 at commit 2e201c2.

jiangxb1987 · 2017-02-08T06:59:04Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

    }
  }

-  test("create hive view for json table") {


These test cases have been moved to HiveSQLViewSuite.

hmmm? these tests are still here

The comment of @jiangxb1987 is out of dated. After leaving this comment, he moved the test cases to SQLViewSuite based on my comments.

jiangxb1987 · 2017-02-08T07:00:29Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

  test("Using view after switching current database") {
    withView("v") {
-      sql("CREATE VIEW v AS SELECT * FROM src")
+      sql("CREATE VIEW v AS SELECT * FROM jt")


Change table src to jt, so we only have to create one single table in beforeAll().

jiangxb1987 · 2017-02-08T07:00:41Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

    }
  }

-  test("create hive view for joined tables") {


These test cases have been moved to HiveSQLViewSuite.

actually you moved it here

jiangxb1987 · 2017-02-08T07:02:37Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

-    }
-  }
-
-  test("correctly resolve a nested view") {


This test case is rewritten to test("correctly parse a nested view")

jiangxb1987 · 2017-02-08T07:03:12Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

-    }
-  }
-
-  test("correctly resolve a view with CTE") {


This test case is duplicated with test("CTE within view")

jiangxb1987 · 2017-02-08T07:03:46Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

-    }
-  }
-
-  test("correctly resolve a view in a self join") {


This test case is rewritten to test("correctly resolve a view in a self join").

jiangxb1987 · 2017-02-08T07:04:57Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

    }
  }

-  test("make sure we can resolve view created by old version of Spark") {


This test case has been moved to HiveSQLViewSuite.

jiangxb1987 · 2017-02-08T07:06:38Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

  test("resolve a view with custom column names") {
-    withTable("testTable") {
-      spark.range(1, 10).selectExpr("id", "id + 1 id1").write.saveAsTable("testTable")
+    withTable("tab1") {


The name testTable has been existed, so we change the table name to tab1.

jiangxb1987 · 2017-02-08T08:10:40Z

retest this please.

SparkQA · 2017-02-08T10:48:02Z

Test build #72573 has finished for PR 16674 at commit 2e201c2.

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

jiangxb1987 · 2017-02-09T01:44:55Z

@gatorsmile @cloud-fan Could you please look at this when you have time? Thanks!

gatorsmile · 2017-02-09T05:56:36Z

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

+    }
+  }
+
+  test("create hive view for joined tables") {


Any reason why this is Hive only?

I moved this to HiveSQLViewSuite just because the test name describes itself to be hive specific.

: ) Could you ignore the test case names and make HiveSQLViewSuite as small as possible?

If we move them to SQLViewSuite, they will also be executed in HiveSQLViewSuite.

Sure - let me do this.

SparkQA · 2017-02-10T04:37:52Z

Test build #72677 has finished for PR 16674 at commit 664b53a.

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

cloud-fan · 2017-02-10T18:22:56Z

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

          u.failAnalysis(s"Table or view not found: ${u.tableName}")
+        // If the database is not valid, also throw an AnalysisException.
+        case _: NoSuchDatabaseException =>
+          u.failAnalysis(s"Table or view not found: ${u.tableName}")


shall we use the qualified name? or users are hard to find out that the database doesn't exist, given only the table name.

Also improved the error message to let users know the database is not found?

cloud-fan · 2017-02-10T18:26:22Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

+        checkAnswer(sql("SELECT * FROM view2 ORDER BY id"), (1 to 9).map(i => Row(i, i)))
+
+        // Alter the referenced view.
+        sql("ALTER VIEW view1 AS SELECT i AS x, j AS y FROM jt2")


nit: ALTER VIEW view1 AS SELECT id AS x, id1 + 1 As y FROM jt, then we don't need to create jt2

cloud-fan · 2017-02-10T18:27:41Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

+      spark.range(1, 10).selectExpr("id", "id + 1 id1").write.saveAsTable("tab1")
+      withView("testView1", "testView2") {
+        // Correctly create a view with custom column names
+        sql("CREATE VIEW testView1(x, y) AS SELECT * FROM tab1")


can we just use jt and avoid creating tab1?

cloud-fan · 2017-02-10T18:35:40Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

    }
  }

  test("resolve a view with custom column names") {


is this test duplicated with https://github.com/apache/spark/pull/16674/files#diff-e09027394aebb11c880b375693f59fafR273 ?

I'm afraid the former test case is on CREATE VIEW, and this test case is on view resolution, so they are not duplicated.

can we merge them? Seems these 2 tests do have some common part.

cloud-fan · 2017-02-10T18:36:06Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

  test("resolve a view when the dataTypes of referenced table columns changed") {
-    withTable("testTable") {
-      spark.range(1, 10).selectExpr("id", "id + 1 id1").write.saveAsTable("testTable")
+    withTable("tab1") {


can we use jt instead?

For this test case I think we'd better create a new table, because we would overwrite the table in the test case, if we use jt then we have to recover the table content and that may be error prone.

cloud-fan · 2017-02-10T18:37:04Z

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

+  override def beforeAll(): Unit = {
+    super.beforeAll()
+    // Create a simple table with two columns: id and id1
+    spark.range(1, 10).selectExpr("id", "id id1").write.format("json").saveAsTable("jt")


can we put this in SQLViewSuite? then we don't need to duplicate this logic in SimpleSQLViewSuite and HiveSQLViewSuite

The problem is that SharedSQLContext also overrides beforeAll() and afterAll(), I haven't figure out a good way to resolve the issue, do you have any suggestions?

what do you mean? In SQLViewSuite.beforeAll, we can still call super.beforeAll

Yes, we can remove both by doing something like

abstract class SQLViewSuite extends QueryTest with SQLTestUtils { import testImplicits._ protected override def beforeAll(): Unit = { super.beforeAll() // Create a simple table with two columns: id and id1 spark.range(1, 10).selectExpr("id", "id id1").write.format("json").saveAsTable("jt") } protected override def afterAll(): Unit = { try { spark.sql(s"DROP TABLE IF EXISTS jt") } finally { super.afterAll() } }

The call of SharedSQLContext.afterAll() is before that of SQLViewSuite.afterAll(), so the variable spark has been set to null in SQLViewSuite.afterAll().

SparkQA · 2017-02-11T20:48:48Z

Test build #72742 has finished for PR 16674 at commit e8fc8c7.

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

cloud-fan · 2017-02-11T22:10:44Z

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

        u: UnresolvedRelation,
        defaultDatabase: Option[String] = None): LogicalPlan = {
+      val tableIdentWithDb = u.tableIdentifier.copy(
+        database = u.tableIdentifier.database.orElse(defaultDatabase))


u.tableIdentifier.database.orElse(defaultDatabase).orElse(Some(conf.defaultDB)), or we can't guarantee we have the database part

val db = u.tableIdentifier.database.orElse(defaultDatabase).orElse(Some(conf.defaultDB))

Then we can use it in the error message.

For a temp view, we should not set the database of the tableIdentifier. Perhaps we can change the case statement to:

case _: NoSuchDatabaseException if db.isDefined => u.failAnalysis(s"Table or view not found: ${tableIdentWithDb.unquotedString}, the " + s"database ${db.get} doesn't exsits.")

gatorsmile · 2017-02-12T07:09:05Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

-    }
-  }
-
-  test("SPARK-14933 - create view from hive parquet table") {


The following removed test cases are moved to HiveSQLViewSuite , because they are Hive specific.

SparkQA · 2017-02-12T13:33:55Z

Test build #72774 has finished for PR 16674 at commit 2f00404.

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

cloud-fan · 2017-02-13T21:47:46Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

 */
-class SQLViewSuite extends QueryTest with SQLTestUtils with TestHiveSingleton {
-  import spark.implicits._
+abstract class SQLViewSuite extends QueryTest with SQLTestUtils {


how about we make a trait SparkSessionProvider and make SharedSparkContext and TestHiveSingleton both extend it? then we can make SQLViewSuite extends SparkSessionProvider and fix this problem.

…fterAll().

SparkQA · 2017-02-14T08:03:05Z

Test build #72853 has finished for PR 16674 at commit 4872918.

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

cloud-fan · 2017-02-14T18:47:52Z

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

+          u.failAnalysis(s"Table or view not found: ${tableIdentWithDb.unquotedString}")
+        // If the database is defined and that database is not found, throw an AnalysisException.
+        // Note that if the database is not defined, it is possible we are looking up a temp view.
+        case _: NoSuchDatabaseException if db.isDefined =>


if users don't specify database, and the current database is removed by other sessions, then they will hit NoSuchDatabaseException. In this case, I think we should also throw AnalysisException for consistency.

let's remove this if, since we can't get the current database in Analyzer

cloud-fan · 2017-02-14T18:48:13Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

+import org.apache.spark.sql.test.{SharedSQLContext, SQLTestUtils}
+
+class SimpleSQLViewSuite extends SQLViewSuite with SharedSQLContext {
+}


the { } is not needed

SparkQA · 2017-02-15T05:00:48Z

Test build #72914 has finished for PR 16674 at commit aa0cf0a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class NoSuchDatabaseException(val db: String) extends AnalysisException(s\"Database '$db' not found\")

cloud-fan · 2017-02-15T18:39:49Z

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

    private def lookupTableFromCatalog(
        u: UnresolvedRelation,
        defaultDatabase: Option[String] = None): LogicalPlan = {
+      val tableIdentWithDb = u.tableIdentifier.copy(database = u.tableIdentifier.database


please follow the existing style: https://github.com/apache/spark/pull/16674/files#diff-57b3d87be744b7d79a9beacf8e5e5eb2L622

nvm, I fixed it while merging

cloud-fan · 2017-02-15T18:47:48Z

thanks, merging to master!

gatorsmile · 2017-02-15T19:06:32Z

A late LGTM : )

Move `SQLViewSuite` from `sql/hive` to `sql/core`, so we can test the view supports without hive metastore. Also moved the test cases that specified to hive to `HiveSQLViewSuite`. Improve the test coverage of SQLViewSuite, cover the following cases: 1. view resolution(possibly a referenced table/view have changed after the view creation); 2. handle a view with user specified column names; 3. improve the test cases for a nested view. Also added a test case for cyclic view reference, which is a known issue that is not fixed yet. N/A Author: jiangxingbo <[email protected]> Closes apache#16674 from jiangxb1987/view-test.

gatorsmile reviewed Jan 22, 2017

View reviewed changes

jiangxb1987 added 2 commits February 8, 2017 13:38

update SQLViewSuite

adde65f

move SQLViewSuite to sql/core.

2e201c2

jiangxb1987 force-pushed the view-test branch from 60d927d to 2e201c2 Compare February 8, 2017 06:56

jiangxb1987 commented Feb 8, 2017

View reviewed changes

gatorsmile reviewed Feb 9, 2017

View reviewed changes

move some test cases.

664b53a

cloud-fan reviewed Feb 10, 2017

View reviewed changes

code refactor.

e8fc8c7

cloud-fan reviewed Feb 11, 2017

View reviewed changes

gatorsmile reviewed Feb 12, 2017

View reviewed changes

merge the beforeAll() and afterAll() logic.

2f00404

cloud-fan reviewed Feb 13, 2017

View reviewed changes

place super.afterAll() before stop SparkContext in SharedSQLContext.a…

4872918

…fterAll().

cloud-fan reviewed Feb 14, 2017

View reviewed changes

code refactor.

aa0cf0a

cloud-fan reviewed Feb 15, 2017

View reviewed changes

asfgit closed this in 3755da7 Feb 15, 2017

jiangxb1987 deleted the view-test branch February 16, 2017 03:03

[SPARK-19331][SQL][TESTS] Improve the test coverage of SQLViewSuite #16674

[SPARK-19331][SQL][TESTS] Improve the test coverage of SQLViewSuite #16674

Uh oh!

Conversation

jiangxb1987 commented Jan 22, 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!

SparkQA commented Jan 22, 2017

Uh oh!

gatorsmile commented Jan 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jan 22, 2017

Uh oh!

jiangxb1987 commented Jan 23, 2017

Uh oh!

gatorsmile commented Jan 26, 2017

Uh oh!

SparkQA commented Feb 8, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented Feb 8, 2017

Uh oh!

SparkQA commented Feb 8, 2017

Uh oh!

jiangxb1987 commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 10, 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

jiangxb1987 commented Jan 22, 2017 •

edited

Loading

gatorsmile Feb 9, 2017 •

edited

Loading

gatorsmile Feb 12, 2017 •

edited

Loading