[SPARK-28628][SQL] Implement SupportsNamespaces in V2SessionCatalog #25363

rdblue · 2019-08-05T16:06:09Z

What changes were proposed in this pull request?

This adds namespace support to V2SessionCatalog.

How was this patch tested?

WIP: will add tests for v2 session catalog namespace methods.

SparkQA · 2019-08-05T19:30:17Z

Test build #108670 has finished for PR 25363 at commit b4f3a93.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class V2SessionCatalog(sessionState: SessionState) extends TableCatalog with SupportsNamespaces

dongjoon-hyun · 2019-08-06T03:24:50Z

For reviewers, the R failure is irrelevant to this PR. (I've hit the same failure at my PR before)

* checking CRAN incoming feasibility ...Error in readRDS(con) : 
  ReadItem: unknown type 0, perhaps written by later version of R
Execution halted
During startup - Warning message:

dongjoon-hyun · 2019-08-06T03:25:02Z

Retest this please.

SparkQA · 2019-08-06T05:59:15Z

Test build #108692 has finished for PR 25363 at commit b4f3a93.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class V2SessionCatalog(sessionState: SessionState) extends TableCatalog with SupportsNamespaces

dongjoon-hyun · 2019-08-08T22:33:53Z

Retest this please.

SparkQA · 2019-08-09T02:18:57Z

Test build #108846 has finished for PR 25363 at commit b4f3a93.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class V2SessionCatalog(sessionState: SessionState) extends TableCatalog with SupportsNamespaces

brkyvz · 2019-08-22T18:00:26Z

The implementation is solid. Looking forward to the tests

rdblue · 2019-08-23T17:06:48Z

@brkyvz, added tests.

SparkQA · 2019-08-23T20:53:15Z

Test build #109649 has finished for PR 25363 at commit c436985.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class V2SessionCatalog(sessionState: SessionState) extends TableCatalog with SupportsNamespaces

brkyvz

A couple extra tests would be great, otherwise LGTM.

brkyvz · 2019-08-26T20:55:45Z

...ore/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala

-    spark.sql("""CREATE DATABASE IF NOT EXISTS ns""")
-    spark.sql("""CREATE DATABASE IF NOT EXISTS ns2""")
+    val catalog = newCatalog()
+    catalog.createNamespace(Array("db"), emptyProps)


can we add a TODO to go through the public APIs once these are implemented.

brkyvz · 2019-08-26T20:56:35Z

...ore/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala

-class V2SessionCatalogSuite
-    extends SparkFunSuite with SharedSparkSession with BeforeAndAfter {
-  import org.apache.spark.sql.catalog.v2.CatalogV2Implicits._
+class V2SessionCatalogSuite extends SparkFunSuite with SharedSparkSession with BeforeAndAfter {


nit: this is no longer a Suite. Can we rename to V2SessionCatalogTestUtils or something?

Updated to V2SessionCatalogBaseSuite because it is the superclass of a suite that provides setup. So more of a suite than just util, but it doesn't contain tests.

brkyvz · 2019-08-26T20:58:28Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

+        // validate that this catalog's reserved properties are not removed
+        changes.foreach {
+          case remove: RemoveProperty
+              if remove.property == "location" || remove.property == "comment" =>


paranoia: does case sensitivity matter here?

I don't think so. There is no guarantee of case insensitivity for table properties implementations.

brkyvz · 2019-08-26T20:59:20Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

+      defaultLocation: Option[URI] = None): CatalogDatabase = {
+    CatalogDatabase(
+      name = db,
+      description = metadata.getOrDefault("comment", ""),


nit: Can we make "comment" and "location" static variables and use those static variables through instead?

...ore/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala

brkyvz · 2019-08-26T21:01:53Z

...ore/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala

+    catalog.dropNamespace(testNs)
+  }
+
+  test("alterNamespace: basic behavior") {


can you also check failing to change database location?

or not sure if that's supported or not. I think not. But you should be able to update the comment (description)

I added tests that fail to remove reserved properties, location and comment, because the catalog requires values. I also added tests that validate they can be updated by altering the namespace to set those properties.

This isn't actually exposed yet because there is no SQL or public API for altering a namespace. But this is how table properties work, so I think it makes sense to match the behavior.

rdblue · 2019-08-31T00:22:37Z

@brkyvz, I updated this to address your comments. When you have a chance, please take another look. Thanks!

SparkQA · 2019-08-31T04:18:10Z

Test build #109980 has finished for PR 25363 at commit 9659f52.

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

brkyvz · 2019-09-01T04:17:34Z

...ore/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala

+
+    assert(initialPath === spark.catalog.getDatabase(testNs(0)).locationUri.toString)
+
+    catalog.alterNamespace(testNs, NamespaceChange.setProperty("location", newPath))


cc @cloud-fan is this hive catalog behavior? I thought you can't change the location of a database.

Hive supports it: https://issues.apache.org/jira/browse/HIVE-8472

However, AFAIK Spark has a problem to support it: #25294

@rdblue can we check if the newly created tables under this namespace can reflect the location change?

Tables are correctly created using the database's location. We have been using this feature for a long time to put all tables for a database in a separate bucket.

cloud-fan · 2019-09-02T05:31:08Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

+      true
+
+    case Array(_) =>
+      // exists returned false


not exists

Correct. This is the case where the database does not exist. We know that because the above existence check returned false. This comment clarifies the Array case because it appears that an Array of one item always matches. So we need to note the context from the previous case.

cloud-fan · 2019-09-03T02:55:17Z

...ore/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala

-class V2SessionCatalogSuite
-    extends SparkFunSuite with SharedSparkSession with BeforeAndAfter {
-  import org.apache.spark.sql.catalog.v2.CatalogV2Implicits._
+class V2SessionCatalogBaseSuite extends SparkFunSuite with SharedSparkSession with BeforeAndAfter {


The session catalog has 2 implementations: in-memory and hive. Shall we follow ExternalCatalogSuite and run the tests in both sql/core and sql/hive?

I'm not sure what you mean. These tests are for the Hive implementation. Applying these to a test v2 session catalog implementation sounds like a different PR to me. Is that what you're suggesting?

yeah seems like this can be done in a follow up

brkyvz · 2019-09-03T20:12:54Z

This LGTM. Merging to master!

rdblue · 2019-09-03T20:19:45Z

Thanks @brkyvz and @cloud-fan for reviewing!

HyukjinKwon mentioned this pull request Aug 6, 2019

[SPARK-28616][INFRA] Improve merge-spark-pr script to warn WIP PRs and strip trailing dots #25356

Closed

dongjoon-hyun added the SQL label Aug 6, 2019

dongjoon-hyun changed the title ~~[SPARK-28628][SQL] Implement SupportsNamespaces in V2SessionCatalog~~ [SPARK-28628][SQL][WIP] Implement SupportsNamespaces in V2SessionCatalog Aug 8, 2019

rdblue force-pushed the SPARK-28628-support-namespaces-in-v2-session-catalog branch from b4f3a93 to c436985 Compare August 23, 2019 17:05

rdblue changed the title ~~[SPARK-28628][SQL][WIP] Implement SupportsNamespaces in V2SessionCatalog~~ [SPARK-28628][SQL] Implement SupportsNamespaces in V2SessionCatalog Aug 23, 2019

brkyvz suggested changes Aug 26, 2019

View reviewed changes

rdblue added 2 commits August 30, 2019 17:16

Support namespaces in V2SessionCatalog.

daf928e

Update tests after review.

9659f52

rdblue force-pushed the SPARK-28628-support-namespaces-in-v2-session-catalog branch from c436985 to 9659f52 Compare August 31, 2019 00:20

brkyvz reviewed Sep 1, 2019

View reviewed changes

cloud-fan reviewed Sep 2, 2019

View reviewed changes

cloud-fan reviewed Sep 3, 2019

View reviewed changes

asfgit closed this in 5ea134c Sep 3, 2019


		assert(initialPath === spark.catalog.getDatabase(testNs(0)).locationUri.toString)

		catalog.alterNamespace(testNs, NamespaceChange.setProperty("location", newPath))

[SPARK-28628][SQL] Implement SupportsNamespaces in V2SessionCatalog #25363

[SPARK-28628][SQL] Implement SupportsNamespaces in V2SessionCatalog #25363

Uh oh!

Conversation

rdblue commented Aug 5, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 5, 2019

Uh oh!

dongjoon-hyun commented Aug 6, 2019

Uh oh!

dongjoon-hyun commented Aug 6, 2019

Uh oh!

SparkQA commented Aug 6, 2019

Uh oh!

dongjoon-hyun commented Aug 8, 2019

Uh oh!

SparkQA commented Aug 9, 2019

Uh oh!

brkyvz commented Aug 22, 2019

Uh oh!

rdblue commented Aug 23, 2019

Uh oh!

SparkQA commented Aug 23, 2019

Uh oh!

brkyvz left a comment

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!

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!

rdblue commented Aug 31, 2019

Uh oh!

SparkQA commented Aug 31, 2019

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!

rdblue Sep 2, 2019 • 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!

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!

brkyvz commented Sep 3, 2019

Uh oh!

rdblue commented Sep 3, 2019

Uh oh!

rdblue Sep 2, 2019 •

edited

Loading