[SPARK-29412][SQL] refine the document of v2 session catalog config #26071

cloud-fan · 2019-10-09T14:33:37Z

What changes were proposed in this pull request?

Refine the document of v2 session catalog config, to clearly explain what it is, when it should be used and how to implement it.

Why are the changes needed?

Make this config more understandable

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the Jenkins with the newly updated test cases.

cloud-fan · 2019-10-09T14:33:52Z

cc @rdblue @brkyvz

cloud-fan · 2019-10-09T14:37:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+        "so that it can delegate calls to Spark Catalog. Otherwise, the implementation " +
+        "should figure out a way to access the Spark Catalog or its underlying meta-store " +
+        "by itself. It's important to make the implementation share the underlying meta-store " +
+        "of the Spark Catalog and act as an extension, instead of a separated catalog.")


This is a complicated config and I'm trying my best to explain it.

cc some SQL guys to get fresh eyes and see if they can understand it. @dongjoon-hyun @viirya @maropu @HyukjinKwon

I think you've identified the right things to point out:

This controls the implementation that is a v2 interface to the built-in v1 catalog

This catalog and the built-in v1 catalog share an identifier namespace and must be consistent

To delegate to the default implementation, use CatalogExtension

I'd change the wording to be a bit shorter, though. This doesn't need to explain what the catalog interface does, or what the built-in catalog can do. How about this?

A catalog implementation that will be used as the v2 interface to Spark's built-in v1 catalog, spark_catalog. This catalog shares its identifier namespace with the v1 Spark catalog and must be consistent with it; for example, if a table can be loaded by the v1 catalog, this catalog must also return the table metadata. To delegate operations to the built-in catalog, implementations can extend CatalogExtension.

thanks for the suggestion!

rdblue · 2019-10-09T16:18:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala

@@ -36,8 +36,7 @@ import org.apache.spark.sql.internal.SQLConf
 */
 // TODO: all commands should look up table from the current catalog. The `SessionCatalog` doesn't
 //       need to track current database at all.
-private[sql]
-class CatalogManager(


Nit: this isn't a related change.

rdblue · 2019-10-09T16:18:52Z

Looks good to me. I'd shorten the config description a bit by removing information that doesn't need to be there and focusing on how the v2 implementation is used by Spark.

imback82

LGTM

rdblue · 2019-10-10T18:05:38Z

+1

Unless there are other comments, I'll merge this later today.

brkyvz · 2019-10-10T18:12:52Z

LGTM as well

dongjoon-hyun · 2019-10-12T23:06:33Z

Retest this please.

dongjoon-hyun · 2019-10-12T23:13:43Z

...src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala

@@ -144,13 +144,13 @@ private [connector] trait SessionCatalogTest[T <: Table, Catalog <: TestV2Sessio
  protected val catalogClassName: String = classOf[InMemoryTableSessionCatalog].getName

  before {
-    spark.conf.set(V2_SESSION_CATALOG.key, catalogClassName)
+    spark.conf.set(V2_SESSION_CATALOG_IMPLEMENTATION.key, catalogClassName)
  }

  override def afterEach(): Unit = {
    super.afterEach()
    catalog("session").asInstanceOf[Catalog].clearTables()


This seems to be last remaining instance.

- catalog("session").asInstanceOf[Catalog].clearTables() + catalog(SESSION_CATALOG_NAME).asInstanceOf[Catalog].clearTables()

This test suite fails without this.

Good catch!

dongjoon-hyun · 2019-10-12T23:18:40Z

...src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala

@@ -26,7 +26,7 @@ import org.apache.spark.sql.catalyst.TableIdentifier
 import org.apache.spark.sql.catalyst.analysis.{NoSuchTableException, TableAlreadyExistsException}
 import org.apache.spark.sql.connector.catalog._


+ import org.apache.spark.sql.connector.catalog.CatalogManager.SESSION_CATALOG_NAME

dongjoon-hyun · 2019-10-12T23:21:46Z

I slightly updated the PR description, @cloud-fan .

dongjoon-hyun · 2019-10-12T23:26:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala

@@ -146,5 +146,5 @@ class CatalogManager(
 }

 private[sql] object CatalogManager {
-  val SESSION_CATALOG_NAME: String = "session"
+  val SESSION_CATALOG_NAME: String = "spark_catalog"


As we see, this cause a test case failure. Could you describe a little bit why we change this in this PR since this is not a documentation change (refine the document of v2 session catalog config).

The config name/document includes the catalog name, so I put them together.

The new name was discussed in a DS v2 community meeting. I'm proposing it in this PR to get more visibility.

dongjoon-hyun · 2019-10-12T23:26:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

-        "passed the Spark built-in session catalog, so that it may delegate calls to the " +
-        "built-in session catalog.")
+  val V2_SESSION_CATALOG_IMPLEMENTATION =
+    buildConf(s"spark.sql.catalog.$SESSION_CATALOG_NAME")


Just a side note, eventually, from user perspective, the exposed configuration names are different.

- spark.sql.catalog.session + spark.sql.catalog.spark_catalog

yes, and it will be hard to change after 3.0 is released.

dongjoon-hyun

+1 for this change, @cloud-fan . Thank you.
I left a few comments including the test failure fixes.

viirya · 2019-10-12T23:32:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala

@@ -77,7 +77,7 @@ class CatalogManager(
  // If the V2_SESSION_CATALOG config is specified, we try to instantiate the user-specified v2


V2_SESSION_CATALOG -? V2_SESSION_CATALOG_IMPLEMENTATION?

viirya · 2019-10-12T23:43:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      .doc("A catalog implementation that will be used as the v2 interface to Spark's built-in " +
+        s"v1 catalog: $SESSION_CATALOG_NAME. This catalog shares its identifier namespace with " +
+        s"the $SESSION_CATALOG_NAME and must be consistent with it; for example, if a table can " +
+        s"be loaded by the $SESSION_CATALOG_NAME, this catalog must also return the table " +
+        s"metadata. To delegate operations to the $SESSION_CATALOG_NAME, implementations can " +
+        "extend 'CatalogExtension'.")


Thanks for pinging me. Almost missing this one.

Does it mean, any implementation configured here, must be the v2 interface to built-in v1 catalog? Sounds like it can delegate or not delegate operations to $SESSION_CATALOG_NAME? If it does not, is it still be the v2 interface to the built-in v1 catalog?

A bit confused here.

It must be a v2 interface, but we can't fully control how it is implemented. We expect it to delegate to the SESSION_CATALOG_NAME, but there is no way to guarantee it.

If the implementation doesn't delegate, then the behavior is undefined.

SparkQA · 2019-10-13T01:49:43Z

Test build #111983 has finished for PR 26071 at commit c54b156.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class CatalogManager(

SparkQA · 2019-10-14T13:02:35Z

Test build #112012 has finished for PR 26071 at commit 18f7ff8.

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

SparkQA · 2019-10-14T17:36:47Z

Test build #112031 has finished for PR 26071 at commit e973e41.

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

cloud-fan · 2019-10-15T02:19:15Z

thanks for the review, merging to master!

dongjoon-hyun · 2019-10-15T02:27:08Z

Thank you!

refine the document of v2 session catalog config

06561bb

cloud-fan commented Oct 9, 2019

View reviewed changes

cloud-fan mentioned this pull request Oct 9, 2019

[SPARK-29374][SQL] do not allow arbitrary v2 catalog as session catalog #26043

Closed

rdblue reviewed Oct 9, 2019

View reviewed changes

dongjoon-hyun added the SQL label Oct 9, 2019

address comments

c54b156

imback82 approved these changes Oct 10, 2019

View reviewed changes

dongjoon-hyun reviewed Oct 12, 2019

View reviewed changes

dongjoon-hyun approved these changes Oct 12, 2019

View reviewed changes

viirya reviewed Oct 12, 2019

View reviewed changes

address comments

18f7ff8

fix test failure due to catalog rename

e973e41

cloud-fan closed this in 9407fba Oct 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-29412][SQL] refine the document of v2 session catalog config #26071

[SPARK-29412][SQL] refine the document of v2 session catalog config #26071

cloud-fan commented Oct 9, 2019 •

edited by dongjoon-hyun

Loading

cloud-fan commented Oct 9, 2019

cloud-fan Oct 9, 2019

rdblue Oct 9, 2019 •

edited

Loading

cloud-fan Oct 10, 2019

rdblue Oct 9, 2019

rdblue commented Oct 9, 2019

imback82 left a comment

rdblue commented Oct 10, 2019

brkyvz commented Oct 10, 2019

dongjoon-hyun commented Oct 12, 2019

dongjoon-hyun Oct 12, 2019

dongjoon-hyun Oct 12, 2019

rdblue Oct 13, 2019

dongjoon-hyun Oct 12, 2019 •

edited

Loading

dongjoon-hyun commented Oct 12, 2019

dongjoon-hyun Oct 12, 2019 •

edited

Loading

cloud-fan Oct 14, 2019

dongjoon-hyun Oct 12, 2019

cloud-fan Oct 14, 2019

dongjoon-hyun left a comment

viirya Oct 12, 2019

viirya Oct 12, 2019

cloud-fan Oct 14, 2019

SparkQA commented Oct 13, 2019

SparkQA commented Oct 14, 2019

SparkQA commented Oct 14, 2019

cloud-fan commented Oct 15, 2019

dongjoon-hyun commented Oct 15, 2019

		@@ -26,7 +26,7 @@ import org.apache.spark.sql.catalyst.TableIdentifier
		import org.apache.spark.sql.catalyst.analysis.{NoSuchTableException, TableAlreadyExistsException}
		import org.apache.spark.sql.connector.catalog._

		@@ -77,7 +77,7 @@ class CatalogManager(
		// If the V2_SESSION_CATALOG config is specified, we try to instantiate the user-specified v2

[SPARK-29412][SQL] refine the document of v2 session catalog config #26071

[SPARK-29412][SQL] refine the document of v2 session catalog config #26071

Conversation

cloud-fan commented Oct 9, 2019 • edited by dongjoon-hyun Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

cloud-fan commented Oct 9, 2019

Choose a reason for hiding this comment

rdblue Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Oct 9, 2019

imback82 left a comment

Choose a reason for hiding this comment

rdblue commented Oct 10, 2019

brkyvz commented Oct 10, 2019

dongjoon-hyun commented Oct 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun Oct 12, 2019 • edited Loading

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 12, 2019

dongjoon-hyun Oct 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 13, 2019

SparkQA commented Oct 14, 2019

SparkQA commented Oct 14, 2019

cloud-fan commented Oct 15, 2019

dongjoon-hyun commented Oct 15, 2019

cloud-fan commented Oct 9, 2019 •

edited by dongjoon-hyun

Loading

rdblue Oct 9, 2019 •

edited

Loading

dongjoon-hyun Oct 12, 2019 •

edited

Loading

dongjoon-hyun Oct 12, 2019 •

edited

Loading