subject:"\[GitHub\] spark pull request #17287\: \[SPARK\-19945\]\[SQL\]add test suite for SessionCatal..."

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-20 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106997366
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withEmptyCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newEmptyCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

Our `reset()` function assumes the existence of Default


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106570569
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withEmptyCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newEmptyCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
   // 
--
   // Databases
   // 
--
 
   test("basic create and list databases") {
-val catalog = new SessionCatalog(newEmptyCatalog())
-catalog.createDatabase(newDb("default"), ignoreIfExists = true)
-assert(catalog.databaseExists("default"))
-assert(!catalog.databaseExists("testing"))
-assert(!catalog.databaseExists("testing2"))
-catalog.createDatabase(newDb("testing"), ignoreIfExists = false)
-assert(catalog.databaseExists("testing"))
-assert(catalog.listDatabases().toSet == Set("default", "testing"))
-catalog.createDatabase(newDb("testing2"), ignoreIfExists = false)
-assert(catalog.listDatabases().toSet == Set("default", "testing", 
"testing2"))
-assert(catalog.databaseExists("testing2"))
-assert(!catalog.databaseExists("does_not_exist"))
+withEmptyCatalog { catalog =>
+  catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

This is a basic test case to test `create and list databases`. Thus, it 
will be good to create a database without the existing one.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106570185
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withEmptyCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newEmptyCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

According to all the test cases in this Suite, it create the default 
database with `ignoreIfExists=true`, we have a default database existed here 
will not effect the test case


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106569991
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withEmptyCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newEmptyCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
   // 
--
   // Databases
   // 
--
 
   test("basic create and list databases") {
-val catalog = new SessionCatalog(newEmptyCatalog())
-catalog.createDatabase(newDb("default"), ignoreIfExists = true)
-assert(catalog.databaseExists("default"))
-assert(!catalog.databaseExists("testing"))
-assert(!catalog.databaseExists("testing2"))
-catalog.createDatabase(newDb("testing"), ignoreIfExists = false)
-assert(catalog.databaseExists("testing"))
-assert(catalog.listDatabases().toSet == Set("default", "testing"))
-catalog.createDatabase(newDb("testing2"), ignoreIfExists = false)
-assert(catalog.listDatabases().toSet == Set("default", "testing", 
"testing2"))
-assert(catalog.databaseExists("testing2"))
-assert(!catalog.databaseExists("does_not_exist"))
+withEmptyCatalog { catalog =>
+  catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

here the ignoreIfExists = true, I think it is ok that the default database 
have already created.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106569958
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

here the `ignoreIfExists = true`, I think it is ok that the default 
database have already created.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread cloud-fan

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106568913
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withEmptyCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newEmptyCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
   // 
--
   // Databases
   // 
--
 
   test("basic create and list databases") {
-val catalog = new SessionCatalog(newEmptyCatalog())
-catalog.createDatabase(newDb("default"), ignoreIfExists = true)
-assert(catalog.databaseExists("default"))
-assert(!catalog.databaseExists("testing"))
-assert(!catalog.databaseExists("testing2"))
-catalog.createDatabase(newDb("testing"), ignoreIfExists = false)
-assert(catalog.databaseExists("testing"))
-assert(catalog.listDatabases().toSet == Set("default", "testing"))
-catalog.createDatabase(newDb("testing2"), ignoreIfExists = false)
-assert(catalog.listDatabases().toSet == Set("default", "testing", 
"testing2"))
-assert(catalog.databaseExists("testing2"))
-assert(!catalog.databaseExists("does_not_exist"))
+withEmptyCatalog { catalog =>
+  catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

this is an example that we should not create default database in 
`withEmptyCatalog`, 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread cloud-fan

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106568853
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withEmptyCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newEmptyCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

shall we do this? Previously when we call `newEmptyCatalog` in test cases, 
there is no default database created


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread cloud-fan

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106568674
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,67 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  private def withBasicCatalog(f: SessionCatalog => Unit): Unit = {
+val catalog = new SessionCatalog(newBasicCatalog())
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
--- End diff --

we already created the default database in `newBasicCatalog`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-16 Thread asfgit

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/17287


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106342082
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -1270,6 +1376,7 @@ class SessionCatalogSuite extends PlanTest {
   }
 
   assert(cause.getMessage.contains("Undefined function: 
'undefined_fn'"))
+  catalog.reset()
--- End diff --

yes, you are right, let me add a try catch


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106340900
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -1270,6 +1376,7 @@ class SessionCatalogSuite extends PlanTest {
   }
 
   assert(cause.getMessage.contains("Undefined function: 
'undefined_fn'"))
+  catalog.reset()
--- End diff --

Then, this `reset()` could be skipped if hitting an exception.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106340219
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -999,257 +1083,279 @@ class SessionCatalogSuite extends PlanTest {
   expectedParts: CatalogTablePartition*): Boolean = {
 // ExternalCatalog may set a default location for partitions, here we 
ignore the partition
 // location when comparing them.
-actualParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet ==
-  expectedParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet
+val actualPartsNormalize = actualParts.map(p =>
--- End diff --

You need to leave a comment to explain it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106336735
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -76,468 +102,500 @@ class SessionCatalogSuite extends PlanTest {
   }
 
   test("create databases using invalid names") {
-val catalog = new SessionCatalog(newEmptyCatalog())
-testInvalidName(name => catalog.createDatabase(newDb(name), 
ignoreIfExists = true))
+withEmptyCatalog { catalog =>
+  testInvalidName(
+name => catalog.createDatabase(newDb(name), ignoreIfExists = true))
+}
   }
 
   test("get database when a database exists") {
-val catalog = new SessionCatalog(newBasicCatalog())
-val db1 = catalog.getDatabaseMetadata("db1")
-assert(db1.name == "db1")
-assert(db1.description.contains("db1"))
+withBasicCatalog { catalog =>
+  val db1 = catalog.getDatabaseMetadata("db1")
+  assert(db1.name == "db1")
+  assert(db1.description.contains("db1"))
+}
   }
 
   test("get database should throw exception when the database does not 
exist") {
-val catalog = new SessionCatalog(newBasicCatalog())
-intercept[NoSuchDatabaseException] {
-  catalog.getDatabaseMetadata("db_that_does_not_exist")
+withBasicCatalog { catalog =>
+  intercept[NoSuchDatabaseException] {
+catalog.getDatabaseMetadata("db_that_does_not_exist")
+  }
 }
   }
 
   test("list databases without pattern") {
-val catalog = new SessionCatalog(newBasicCatalog())
-assert(catalog.listDatabases().toSet == Set("default", "db1", "db2", 
"db3"))
+withBasicCatalog { catalog =>
+  assert(catalog.listDatabases().toSet == Set("default", "db1", "db2", 
"db3"))
+}
   }
 
   test("list databases with pattern") {
-val catalog = new SessionCatalog(newBasicCatalog())
-assert(catalog.listDatabases("db").toSet == Set.empty)
-assert(catalog.listDatabases("db*").toSet == Set("db1", "db2", "db3"))
-assert(catalog.listDatabases("*1").toSet == Set("db1"))
-assert(catalog.listDatabases("db2").toSet == Set("db2"))
+withBasicCatalog { catalog =>
+  assert(catalog.listDatabases("db").toSet == Set.empty)
+  assert(catalog.listDatabases("db*").toSet == Set("db1", "db2", 
"db3"))
+  assert(catalog.listDatabases("*1").toSet == Set("db1"))
+  assert(catalog.listDatabases("db2").toSet == Set("db2"))
+}
   }
 
   test("drop database") {
-val catalog = new SessionCatalog(newBasicCatalog())
-catalog.dropDatabase("db1", ignoreIfNotExists = false, cascade = false)
-assert(catalog.listDatabases().toSet == Set("default", "db2", "db3"))
+withBasicCatalog { catalog =>
+  catalog.dropDatabase("db1", ignoreIfNotExists = false, cascade = 
false)
+  assert(catalog.listDatabases().toSet == Set("default", "db2", "db3"))
+}
   }
 
   test("drop database when the database is not empty") {
 // Throw exception if there are functions left
-val externalCatalog1 = newBasicCatalog()
-val sessionCatalog1 = new SessionCatalog(externalCatalog1)
-externalCatalog1.dropTable("db2", "tbl1", ignoreIfNotExists = false, 
purge = false)
-externalCatalog1.dropTable("db2", "tbl2", ignoreIfNotExists = false, 
purge = false)
-intercept[AnalysisException] {
-  sessionCatalog1.dropDatabase("db2", ignoreIfNotExists = false, 
cascade = false)
+withBasicCatalog { catalog =>
+  catalog.externalCatalog.dropTable("db2", "tbl1", ignoreIfNotExists = 
false, purge = false)
+  catalog.externalCatalog.dropTable("db2", "tbl2", ignoreIfNotExists = 
false, purge = false)
+  intercept[AnalysisException] {
+catalog.dropDatabase("db2", ignoreIfNotExists = false, cascade = 
false)
+  }
 }
-
-// Throw exception if there are tables left
-val externalCatalog2 = newBasicCatalog()
-val sessionCatalog2 = new SessionCatalog(externalCatalog2)
-externalCatalog2.dropFunction("db2", "func1")
-intercept[AnalysisException] {
-  sessionCatalog2.dropDatabase("db2", ignoreIfNotExists = false, 
cascade = false)
+withBasicCatalog { catalog =>
+  // Throw exception if there are tables left
+  catalog.externalCatalog.dropFunction("db2", "func1")
+  intercept[AnalysisException] {
+catalog.dropDatabase("db2", ignoreIfNotExists = false, cascade = 
false)
+  }
 }
 
-// When cascade is true, it should drop them
-val externalCatalog3 = newBasicCatalog()
-val sessionCatalog3 = new SessionCatalog(externalCatalog3)

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106336045
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -999,257 +1083,279 @@ class SessionCatalogSuite extends PlanTest {
   expectedParts: CatalogTablePartition*): Boolean = {
 // ExternalCatalog may set a default location for partitions, here we 
ignore the partition
 // location when comparing them.
-actualParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet ==
-  expectedParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet
+val actualPartsNormalize = actualParts.map(p =>
+  p.copy(parameters = Map.empty, storage = p.storage.copy(
+properties = Map.empty, locationUri = None, serde = None))).toSet
+
+val expectedPartsNormalize = expectedParts.map(p =>
+p.copy(parameters = Map.empty, storage = p.storage.copy(
+  properties = Map.empty, locationUri = None, serde = None))).toSet
+
+actualPartsNormalize == expectedPartsNormalize
+//actualParts.map(p =>
+//  p.copy(storage = p.storage.copy(
+//properties = Map.empty, locationUri = None))).toSet ==
+//  expectedParts.map(p =>
+//p.copy(storage = p.storage.copy(properties = Map.empty, 
locationUri = None))).toSet
--- End diff --

sorry, let me remove it


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106335967
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -1270,6 +1376,7 @@ class SessionCatalogSuite extends PlanTest {
   }
 
   assert(cause.getMessage.contains("Undefined function: 
'undefined_fn'"))
+  catalog.reset()
--- End diff --

here the `SessionCatalog` is instanced with different `conf` parameter.
In `withBasicCatalog`, it just leave it default


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106335778
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -999,257 +1083,279 @@ class SessionCatalogSuite extends PlanTest {
   expectedParts: CatalogTablePartition*): Boolean = {
 // ExternalCatalog may set a default location for partitions, here we 
ignore the partition
 // location when comparing them.
-actualParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet ==
-  expectedParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet
+val actualPartsNormalize = actualParts.map(p =>
--- End diff --

Yes, it is~


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106334939
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -999,257 +1083,279 @@ class SessionCatalogSuite extends PlanTest {
   expectedParts: CatalogTablePartition*): Boolean = {
 // ExternalCatalog may set a default location for partitions, here we 
ignore the partition
 // location when comparing them.
-actualParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet ==
-  expectedParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet
+val actualPartsNormalize = actualParts.map(p =>
--- End diff --

Because Hive metastore fills the values after we calling the Hive APIs? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106334827
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -999,257 +1094,279 @@ class SessionCatalogSuite extends PlanTest {
   expectedParts: CatalogTablePartition*): Boolean = {
 // ExternalCatalog may set a default location for partitions, here we 
ignore the partition
 // location when comparing them.
-actualParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet ==
-  expectedParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet
+val actualPartsNormalize = actualParts.map(p =>
--- End diff --

Because Hive metastore fills the values after we call the Hive APIs?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106334626
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -999,257 +1083,279 @@ class SessionCatalogSuite extends PlanTest {
   expectedParts: CatalogTablePartition*): Boolean = {
 // ExternalCatalog may set a default location for partitions, here we 
ignore the partition
 // location when comparing them.
-actualParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet ==
-  expectedParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet
+val actualPartsNormalize = actualParts.map(p =>
+  p.copy(parameters = Map.empty, storage = p.storage.copy(
+properties = Map.empty, locationUri = None, serde = None))).toSet
+
+val expectedPartsNormalize = expectedParts.map(p =>
+p.copy(parameters = Map.empty, storage = p.storage.copy(
+  properties = Map.empty, locationUri = None, serde = None))).toSet
+
+actualPartsNormalize == expectedPartsNormalize
+//actualParts.map(p =>
+//  p.copy(storage = p.storage.copy(
+//properties = Map.empty, locationUri = None))).toSet ==
+//  expectedParts.map(p =>
+//p.copy(storage = p.storage.copy(properties = Map.empty, 
locationUri = None))).toSet
--- End diff --

?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106334485
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -1270,6 +1376,7 @@ class SessionCatalogSuite extends PlanTest {
   }
 
   assert(cause.getMessage.contains("Undefined function: 
'undefined_fn'"))
+  catalog.reset()
--- End diff --

Instead of adding `reset`, why not using your new function 
`withBasicCatalog`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106333835
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalSessionCatalogSuite.scala
 ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hive
+
+import org.apache.spark.sql.catalyst.catalog.{CatalogTestUtils, 
ExternalCatalog, SessionCatalogSuite}
+import org.apache.spark.sql.hive.test.TestHiveSingleton
+
+class HiveExternalSessionCatalogSuite extends SessionCatalogSuite with 
TestHiveSingleton {
+
+  protected override val isHiveExternalCatalog = true
+
+  private val externalCatalog = {
+val catalog = spark.sharedState.externalCatalog
+catalog.asInstanceOf[HiveExternalCatalog].client.reset()
+catalog
+  }
+
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"org.apache.hadoop.mapred.SequenceFileInputFormat"
+override val tableOutputFormat: String =
+  "org.apache.hadoop.hive.ql.io.HiveSequenceFileOutputFormat"
+override val defaultProvider: String = "parquet"
--- End diff --

The above input and output formats does not match what you specified here. 
Let us change it to `hive`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106145746
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,83 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  val BASIC = "basic"
+  val EMPTY = "empty"
+
+  private def withSessionCatalog(
--- End diff --

ok~ thanks~


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread cloud-fan

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106093186
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,83 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  val BASIC = "basic"
+  val EMPTY = "empty"
+
+  private def withSessionCatalog(
+  catalogTpe: String = BASIC)(f: SessionCatalog => Unit): Unit = {
+val catalog = catalogTpe match {
+  case BASIC =>
+new SessionCatalog(newBasicCatalog())
+  case EMPTY =>
+new SessionCatalog(newEmptyCatalog())
+}
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withSessionCatalogAndExternal(
--- End diff --

yea we should just access the external catalog


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-15 Thread cloud-fan

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106093083
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,83 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  val BASIC = "basic"
+  val EMPTY = "empty"
+
+  private def withSessionCatalog(
--- End diff --

I'd like to have 2 methods: `withBasicCatalog` and `withEmptyCatalog`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106065119
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -76,468 +118,499 @@ class SessionCatalogSuite extends PlanTest {
   }
 
   test("create databases using invalid names") {
-val catalog = new SessionCatalog(newEmptyCatalog())
-testInvalidName(name => catalog.createDatabase(newDb(name), 
ignoreIfExists = true))
+withSessionCatalog(EMPTY) { catalog =>
+  testInvalidName(
+name => catalog.createDatabase(newDb(name), ignoreIfExists = true))
+}
   }
 
   test("get database when a database exists") {
-val catalog = new SessionCatalog(newBasicCatalog())
-val db1 = catalog.getDatabaseMetadata("db1")
-assert(db1.name == "db1")
-assert(db1.description.contains("db1"))
+withSessionCatalog() { catalog =>
+  val db1 = catalog.getDatabaseMetadata("db1")
+  assert(db1.name == "db1")
+  assert(db1.description.contains("db1"))
+}
   }
 
   test("get database should throw exception when the database does not 
exist") {
-val catalog = new SessionCatalog(newBasicCatalog())
-intercept[NoSuchDatabaseException] {
-  catalog.getDatabaseMetadata("db_that_does_not_exist")
+withSessionCatalog() { catalog =>
+  intercept[NoSuchDatabaseException] {
+catalog.getDatabaseMetadata("db_that_does_not_exist")
+  }
 }
   }
 
   test("list databases without pattern") {
-val catalog = new SessionCatalog(newBasicCatalog())
-assert(catalog.listDatabases().toSet == Set("default", "db1", "db2", 
"db3"))
+withSessionCatalog() { catalog =>
+  assert(catalog.listDatabases().toSet == Set("default", "db1", "db2", 
"db3"))
+}
   }
 
   test("list databases with pattern") {
-val catalog = new SessionCatalog(newBasicCatalog())
-assert(catalog.listDatabases("db").toSet == Set.empty)
-assert(catalog.listDatabases("db*").toSet == Set("db1", "db2", "db3"))
-assert(catalog.listDatabases("*1").toSet == Set("db1"))
-assert(catalog.listDatabases("db2").toSet == Set("db2"))
+withSessionCatalog() { catalog =>
+  assert(catalog.listDatabases("db").toSet == Set.empty)
+  assert(catalog.listDatabases("db*").toSet == Set("db1", "db2", 
"db3"))
+  assert(catalog.listDatabases("*1").toSet == Set("db1"))
+  assert(catalog.listDatabases("db2").toSet == Set("db2"))
+}
   }
 
   test("drop database") {
-val catalog = new SessionCatalog(newBasicCatalog())
-catalog.dropDatabase("db1", ignoreIfNotExists = false, cascade = false)
-assert(catalog.listDatabases().toSet == Set("default", "db2", "db3"))
+withSessionCatalog() { catalog =>
+  catalog.dropDatabase("db1", ignoreIfNotExists = false, cascade = 
false)
+  assert(catalog.listDatabases().toSet == Set("default", "db2", "db3"))
+}
   }
 
   test("drop database when the database is not empty") {
 // Throw exception if there are functions left
-val externalCatalog1 = newBasicCatalog()
-val sessionCatalog1 = new SessionCatalog(externalCatalog1)
-externalCatalog1.dropTable("db2", "tbl1", ignoreIfNotExists = false, 
purge = false)
-externalCatalog1.dropTable("db2", "tbl2", ignoreIfNotExists = false, 
purge = false)
-intercept[AnalysisException] {
-  sessionCatalog1.dropDatabase("db2", ignoreIfNotExists = false, 
cascade = false)
+withSessionCatalogAndExternal() { (catalog, externalCatalog) =>
+  externalCatalog.dropTable("db2", "tbl1", ignoreIfNotExists = false, 
purge = false)
+  externalCatalog.dropTable("db2", "tbl2", ignoreIfNotExists = false, 
purge = false)
+  intercept[AnalysisException] {
+catalog.dropDatabase("db2", ignoreIfNotExists = false, cascade = 
false)
+  }
 }
-
-// Throw exception if there are tables left
-val externalCatalog2 = newBasicCatalog()
-val sessionCatalog2 = new SessionCatalog(externalCatalog2)
-externalCatalog2.dropFunction("db2", "func1")
-intercept[AnalysisException] {
-  sessionCatalog2.dropDatabase("db2", ignoreIfNotExists = false, 
cascade = false)
+withSessionCatalogAndExternal() { (catalog, externalCatalog) =>
+  // Throw exception if there are tables left
+  externalCatalog.dropFunction("db2", "func1")
+  intercept[AnalysisException] {
+catalog.dropDatabase("db2", ignoreIfNotExists = false, cascade = 
false)
+  }
 }
 
-// When cascade is true, it should drop them
-val externalCatalog3 =

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106020447
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -76,468 +118,499 @@ class SessionCatalogSuite extends PlanTest {
   }
 
   test("create databases using invalid names") {
-val catalog = new SessionCatalog(newEmptyCatalog())
-testInvalidName(name => catalog.createDatabase(newDb(name), 
ignoreIfExists = true))
+withSessionCatalog(EMPTY) { catalog =>
+  testInvalidName(
+name => catalog.createDatabase(newDb(name), ignoreIfExists = true))
+}
   }
 
   test("get database when a database exists") {
-val catalog = new SessionCatalog(newBasicCatalog())
-val db1 = catalog.getDatabaseMetadata("db1")
-assert(db1.name == "db1")
-assert(db1.description.contains("db1"))
+withSessionCatalog() { catalog =>
+  val db1 = catalog.getDatabaseMetadata("db1")
+  assert(db1.name == "db1")
+  assert(db1.description.contains("db1"))
+}
   }
 
   test("get database should throw exception when the database does not 
exist") {
-val catalog = new SessionCatalog(newBasicCatalog())
-intercept[NoSuchDatabaseException] {
-  catalog.getDatabaseMetadata("db_that_does_not_exist")
+withSessionCatalog() { catalog =>
+  intercept[NoSuchDatabaseException] {
+catalog.getDatabaseMetadata("db_that_does_not_exist")
+  }
 }
   }
 
   test("list databases without pattern") {
-val catalog = new SessionCatalog(newBasicCatalog())
-assert(catalog.listDatabases().toSet == Set("default", "db1", "db2", 
"db3"))
+withSessionCatalog() { catalog =>
+  assert(catalog.listDatabases().toSet == Set("default", "db1", "db2", 
"db3"))
+}
   }
 
   test("list databases with pattern") {
-val catalog = new SessionCatalog(newBasicCatalog())
-assert(catalog.listDatabases("db").toSet == Set.empty)
-assert(catalog.listDatabases("db*").toSet == Set("db1", "db2", "db3"))
-assert(catalog.listDatabases("*1").toSet == Set("db1"))
-assert(catalog.listDatabases("db2").toSet == Set("db2"))
+withSessionCatalog() { catalog =>
+  assert(catalog.listDatabases("db").toSet == Set.empty)
+  assert(catalog.listDatabases("db*").toSet == Set("db1", "db2", 
"db3"))
+  assert(catalog.listDatabases("*1").toSet == Set("db1"))
+  assert(catalog.listDatabases("db2").toSet == Set("db2"))
+}
   }
 
   test("drop database") {
-val catalog = new SessionCatalog(newBasicCatalog())
-catalog.dropDatabase("db1", ignoreIfNotExists = false, cascade = false)
-assert(catalog.listDatabases().toSet == Set("default", "db2", "db3"))
+withSessionCatalog() { catalog =>
+  catalog.dropDatabase("db1", ignoreIfNotExists = false, cascade = 
false)
+  assert(catalog.listDatabases().toSet == Set("default", "db2", "db3"))
+}
   }
 
   test("drop database when the database is not empty") {
 // Throw exception if there are functions left
-val externalCatalog1 = newBasicCatalog()
-val sessionCatalog1 = new SessionCatalog(externalCatalog1)
-externalCatalog1.dropTable("db2", "tbl1", ignoreIfNotExists = false, 
purge = false)
-externalCatalog1.dropTable("db2", "tbl2", ignoreIfNotExists = false, 
purge = false)
-intercept[AnalysisException] {
-  sessionCatalog1.dropDatabase("db2", ignoreIfNotExists = false, 
cascade = false)
+withSessionCatalogAndExternal() { (catalog, externalCatalog) =>
+  externalCatalog.dropTable("db2", "tbl1", ignoreIfNotExists = false, 
purge = false)
+  externalCatalog.dropTable("db2", "tbl2", ignoreIfNotExists = false, 
purge = false)
+  intercept[AnalysisException] {
+catalog.dropDatabase("db2", ignoreIfNotExists = false, cascade = 
false)
+  }
 }
-
-// Throw exception if there are tables left
-val externalCatalog2 = newBasicCatalog()
-val sessionCatalog2 = new SessionCatalog(externalCatalog2)
-externalCatalog2.dropFunction("db2", "func1")
-intercept[AnalysisException] {
-  sessionCatalog2.dropDatabase("db2", ignoreIfNotExists = false, 
cascade = false)
+withSessionCatalogAndExternal() { (catalog, externalCatalog) =>
+  // Throw exception if there are tables left
+  externalCatalog.dropFunction("db2", "func1")
+  intercept[AnalysisException] {
+catalog.dropDatabase("db2", ignoreIfNotExists = false, cascade = 
false)
+  }
 }
 
-// When cascade is true, it should drop them
-val externalCatalog3 =

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106020015
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,83 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  val BASIC = "basic"
+  val EMPTY = "empty"
+
+  private def withSessionCatalog(
+  catalogTpe: String = BASIC)(f: SessionCatalog => Unit): Unit = {
+val catalog = catalogTpe match {
+  case BASIC =>
+new SessionCatalog(newBasicCatalog())
+  case EMPTY =>
+new SessionCatalog(newEmptyCatalog())
+}
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withSessionCatalogAndExternal(
--- End diff --

cc @cloud-fan to confirm this solution is ok


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106019799
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,83 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  val BASIC = "basic"
+  val EMPTY = "empty"
+
+  private def withSessionCatalog(
+  catalogTpe: String = BASIC)(f: SessionCatalog => Unit): Unit = {
+val catalog = catalogTpe match {
+  case BASIC =>
+new SessionCatalog(newBasicCatalog())
+  case EMPTY =>
+new SessionCatalog(newEmptyCatalog())
+}
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
+}
+  }
+
+  private def withSessionCatalogAndExternal(
--- End diff --

This function is not needed, I think. 

We just need to make ExternalCatalog accessible. Then, the test case can 
directly access it by `catalog.externalCatalog`
```
class SessionCatalog(
val externalCatalog: ExternalCatalog,
globalTempViewManager: GlobalTempViewManager,
functionRegistry: FunctionRegistry,
conf: CatalystConf,
hadoopConf: Configuration,
parser: ParserInterface) 
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread gatorsmile

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r106017976
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,83 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  val BASIC = "basic"
+  val EMPTY = "empty"
--- End diff --

These should be moved to `CatalogTestUtils`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r105860834
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -999,257 +1094,279 @@ class SessionCatalogSuite extends PlanTest {
   expectedParts: CatalogTablePartition*): Boolean = {
 // ExternalCatalog may set a default location for partitions, here we 
ignore the partition
 // location when comparing them.
-actualParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet ==
-  expectedParts.map(p => p.copy(storage = p.storage.copy(locationUri = 
None))).toSet
+val actualPartsNormalize = actualParts.map(p =>
--- End diff --

this is the expected part.

https://github.com/apache/spark/blob/master/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogSuite.scala#L857

here add some normalize logic


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread windpiger

Github user windpiger commented on a diff in the pull request:

https://github.com/apache/spark/pull/17287#discussion_r105858890
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -27,41 +27,83 @@ import 
org.apache.spark.sql.catalyst.parser.CatalystSqlParser
 import org.apache.spark.sql.catalyst.plans.PlanTest
 import org.apache.spark.sql.catalyst.plans.logical.{Range, SubqueryAlias, 
View}
 
+class InMemorySessionCatalogSuite extends SessionCatalogSuite {
+  protected val utils = new CatalogTestUtils {
+override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
+override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
+override val defaultProvider: String = "parquet"
+override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
+  }
+}
+
 /**
- * Tests for [[SessionCatalog]] that assume that [[InMemoryCatalog]] is 
correctly implemented.
+ * Tests for [[SessionCatalog]]
  *
  * Note: many of the methods here are very similar to the ones in 
[[ExternalCatalogSuite]].
  * This is because [[SessionCatalog]] and [[ExternalCatalog]] share many 
similar method
  * signatures but do not extend a common parent. This is largely by design 
but
  * unfortunately leads to very similar test code in two places.
  */
-class SessionCatalogSuite extends PlanTest {
-  private val utils = new CatalogTestUtils {
-override val tableInputFormat: String = 
"com.fruit.eyephone.CameraInputFormat"
-override val tableOutputFormat: String = 
"com.fruit.eyephone.CameraOutputFormat"
-override val defaultProvider: String = "parquet"
-override def newEmptyCatalog(): ExternalCatalog = new InMemoryCatalog
-  }
+abstract class SessionCatalogSuite extends PlanTest {
+  protected val utils: CatalogTestUtils
+
+  protected val isHiveExternalCatalog = false
 
   import utils._
 
+  val BASIC = "basic"
+  val EMPTY = "empty"
+
+  private def withSessionCatalog(
+  catalogTpe: String = BASIC)(f: SessionCatalog => Unit): Unit = {
+val catalog = catalogTpe match {
+  case BASIC =>
+new SessionCatalog(newBasicCatalog())
+  case EMPTY =>
+new SessionCatalog(newEmptyCatalog())
+}
+catalog.createDatabase(newDb("default"), ignoreIfExists = true)
+try {
+  f(catalog)
+} finally {
+  catalog.reset()
--- End diff --

reset the catalog


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

2017-03-14 Thread windpiger

GitHub user windpiger opened a pull request:

https://github.com/apache/spark/pull/17287

[SPARK-19945][SQL]add test suite for SessionCatalog with HiveExternalCatalog


## What changes were proposed in this pull request?

Currently `SessionCatalogSuite` is only for `InMemoryCatalog`, there is no 
suite for `HiveExternalCatalog`.
And there are some ddl function is not proper to test in 
`ExternalCatalogSuite`, because some logic are not full implement in 
`ExternalCatalog`, these ddl functions are full implement in `SessionCatalog`, 
it is better to test it in `SessionCatalogSuite`

So we should add a test suite for `SessionCatalog` with 
`HiveExternalCatalog`
## How was this patch tested?
add `HiveExternalSessionCatalogSuite`

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/windpiger/spark sessioncatalogsuit

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/17287.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #17287


commit 44684461c86d2810ffdfae23485d74fba3801e12
Author: windpiger 
Date:   2017-03-14T09:11:57Z

[SPARK-19945][SQL]add test suite for SessionCatalog with HiveExternalCatalog




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

[GitHub] spark pull request #17287: [SPARK-19945][SQL]add test suite for SessionCatal...

32 matches

Site Navigation

Mail list logo

Footer information