[SPARK-28341][SQL] create a public API for V2SessionCatalog #25104

cloud-fan · 2019-07-11T01:47:07Z

What changes were proposed in this pull request?

The V2SessionCatalog has 2 functionalities:

work as an adapter: provide v2 APIs and translate calls to the SessionCatalog.
allow users to extend it, so that they can add hooks to apply custom logic before calling methods of the builtin catalog (session catalog).

To leverage the second functionality, users must extend V2SessionCatalog which is an internal class. There is no doc to explain this usage.

This PR does 2 things:

refine the document of the config spark.sql.catalog.session.
add a public abstract class CatalogExtension for users to write implementations.

TODOs for followup PRs:

discuss if we should allow users to completely overwrite the v2 session catalog with a new one.
discuss to change the name of session catalog, so that it's less likely to conflict with existing namespace names.

How was this patch tested?

existing tests

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

cloud-fan · 2019-07-11T01:52:20Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

Now this end-to-end test checks the actual behavior the end-users will hit, as we don't set a fake session catalog.

cloud-fan · 2019-07-11T01:53:39Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

change the test name a little bit, because we will always use default catalog when the config is set, no matter what the table provider is.

cloud-fan · 2019-07-11T01:54:26Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

Note that, this is never supported. The test can pass before because we set a fake session catalog which returns writable tables.

To clarify, this doesn't work because the ORC source is broken.

The test case is still valid, so I don't see a reason why we should get rid of it. I also think that it is a valid use to override the v2 session catalog implementation, which this relies on for tests.

cloud-fan · 2019-07-11T01:55:52Z

cc @rdblue @brkyvz @jose-torres @jzhuge

SparkQA · 2019-07-11T05:27:58Z

Test build #107498 has finished for PR 25104 at commit cba0e25.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class CatalogManager(conf: SQLConf, val v2SessionCatalog: TableCatalog)
class NoopV2SessionCatalog extends TableCatalog
class V2SessionCatalog(catalog: SessionCatalog, conf: SQLConf) extends TableCatalog

rdblue · 2019-07-11T17:06:29Z

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

Why use the Java-like "get"? I think def catalog(name: String) is sufficient.

I try to avoid adding get to names. It doesn't help understanding unless the method is a getter -- and this isn't -- so why include an extra word?

rdblue · 2019-07-11T17:06:42Z

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

Same here. No need for get.

rdblue · 2019-07-11T17:13:18Z

This looks okay overall, but it breaks the test for v2 providers with the session catalog. I don't think this can be committed until that is resolved. @cloud-fan, can you test whether one of the test providers will work in place of ORC v2 in this test?

The second issue is that this removes the ability to override the v2 session catalog implementation. I think we want to support that so you can modify the behavior of the v2 session catalog, but continue to use the v1 catalog for v1 cases. @cloud-fan, do you have any idea about how to make this possible?

I think @brkyvz is interested in overriding the v2 session catalog as well.

brkyvz

@rdblue I still have some confusion around the default.catalog conf and catalog.session.

spark.sql.default.catalog: Name of the default v2 catalog, used when a catalog is not identified in queries
spark.sql.catalog.session: Name of the default v2 catalog, used when a catalog is not identified in queries.

The description of both is identical. The default v2 catalog can be configured on a session basis just like the v2_session_catalog. Why can't DEFAULT_V2_CATALOG be used to configure the session catalog?

brkyvz · 2019-07-11T18:02:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalog/v2/LookupCatalog.scala

The session catalog needs to be configurable. This is how custom data sources / table formats will plugin.

brkyvz · 2019-07-11T18:04:06Z

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

Since the nice error message handling is gone, if there's an error, what does it look like?

the error message handling is in defaultCatalog() and v2SessionCatalog.

rdblue · 2019-07-11T18:56:23Z

@brkyvz, the description must have been copied by mistake. The session property controls what implementation we use for the v2 session catalog. See my comment above for the difference between the v2 session catalog and the default catalog.

brkyvz · 2019-07-12T15:03:25Z

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

Why does this need to be a StaticConf? Wouldn't this make it impossible to plug in your own session catalog when using the Spark Shell for example?

I think it makes sense to make this a static conf because the v2 session catalog cannot change after a session loads it the first time. Static confs can still be set using --conf right?

SparkQA · 2019-07-12T16:23:12Z

Test build #107596 has finished for PR 25104 at commit 4f89b88.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class CatalogManager(conf: SQLConf, val sessionCatalog: SessionCatalog) extends Logging
.doc(\"The implementation class of the v2 session catalog, which is a wrapper of the internal\" +
class V2SessionCatalog extends TableCatalog with RequiresSessionCatalog

rdblue · 2019-07-12T16:31:32Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/RequiresSessionCatalog.java

This has a method, so it isn't a marker interface.

This should note that this is for implementations that want to use Spark's session catalog for storage, which is needed for writing replacements for V2SessionCatalog. Maybe also mention that this will go away when the SessionCatalog is removed.

rdblue · 2019-07-12T16:32:13Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/RequiresSessionCatalog.java

If this doesn't replace the existing catalog initialization method, then it should have a different name. How about setSessionCatalog?

rdblue · 2019-07-12T16:38:03Z

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

This should not fall back to using an implementation other than the one the user configured. If the configured v2 session catalog cannot be instantiated, then the error should be thrown each time there is an attempt to use it.

rdblue · 2019-07-12T16:39:00Z

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

This should not swallow all non-fatal errors. It should catch CatalogNotFoundException only.

SparkQA · 2019-07-12T17:12:58Z

Test build #107598 has finished for PR 25104 at commit 5f0c5e1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class CatalogManager(conf: SQLConf, val sessionCatalog: SessionCatalog) extends Logging
.doc(\"The implementation class of the v2 session catalog, which is a wrapper of the internal\" +
class V2SessionCatalog extends TableCatalog with RequiresSessionCatalog

rdblue · 2019-07-12T17:39:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalog/v2/LookupCatalog.scala

Why is this TableCatalog and not CatalogPlugin?

rdblue · 2019-07-12T17:45:29Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

This should reference a JIRA issue to track the fix.

rdblue · 2019-07-12T17:46:05Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

This should use the original test case to minimize changes.

rdblue · 2019-07-12T17:48:33Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

It is odd that this requires passing in the session when getTestCatalog does not. I would prefer not using this function in places where the session is different.

rdblue · 2019-07-12T17:54:23Z

sql/core/src/main/scala/org/apache/spark/sql/internal/BaseSessionStateBuilder.scala

Is this import used? Same with V2SessionCatalog. And check HiveSessionStateBuilder, too.

SparkQA · 2019-09-05T06:54:56Z

Test build #110162 has finished for PR 25104 at commit 155c37f.

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

SparkQA · 2019-09-05T09:35:24Z

Test build #110169 has finished for PR 25104 at commit 75e2ca7.

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

SparkQA · 2019-09-05T11:09:03Z

Test build #110170 has finished for PR 25104 at commit f89f25a.

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

SparkQA · 2019-09-05T16:45:29Z

Test build #110184 has finished for PR 25104 at commit 494e031.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2019-09-06T07:05:02Z

Test build #110218 has finished for PR 25104 at commit 8240062.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-09-06T14:17:11Z

retest this please

brkyvz · 2019-09-06T18:10:30Z

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

+      try {
+        catalogs.getOrElseUpdate(CatalogManager.SESSION_CATALOG_NAME, loadV2SessionCatalog())
+      } catch {
+        case NonFatal(_) => defaultSessionCatalog


I'd log the error. The user asked for a specific catalog, but we're giving them the default. There's no way to figure out the discrepancy without having the error

brkyvz

LGTM except the one case around logging the error. Should we also make V2SessionCatalog private[spark] after this case?

SparkQA · 2019-09-06T18:30:46Z

Test build #110248 has finished for PR 25104 at commit 8240062.

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

brkyvz · 2019-09-06T19:14:39Z

Another question is, some guidance on how a StagingTableCatalog can leverage the CatalogExtension API.

cloud-fan · 2019-09-09T04:33:50Z

If V2SessionCatalog implements StagingTableCatalog one day, it should still call createTable, dropTable, etc. somewhere which can be extended by CatalogExtension.

If users want to override the staging logic, they can implement StagingTableCatalog themselves.

SparkQA · 2019-09-09T07:05:01Z

Test build #110328 has finished for PR 25104 at commit 05db860.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-09-09T07:07:55Z

retest this please

SparkQA · 2019-09-09T11:02:31Z

Test build #110336 has finished for PR 25104 at commit 05db860.

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

cloud-fan · 2019-09-09T13:15:05Z

thanks for the review, merging to master!

cloud-fan · 2019-09-09T13:47:46Z

BTW, V2SessionCatalog is in the execution package, which is meant to be private, like the catalyst package.

rdblue · 2019-09-11T16:44:38Z

@cloud-fan, if execution is intended to be private, should we move those classes into catalyst?

cloud-fan · 2019-09-11T16:50:32Z

We can do it if V2SessionCatalog doesn't refer to anything in sql/core.

## What changes were proposed in this pull request? The `V2SessionCatalog` has 2 functionalities: 1. work as an adapter: provide v2 APIs and translate calls to the `SessionCatalog`. 2. allow users to extend it, so that they can add hooks to apply custom logic before calling methods of the builtin catalog (session catalog). To leverage the second functionality, users must extend `V2SessionCatalog` which is an internal class. There is no doc to explain this usage. This PR does 2 things: 1. refine the document of the config `spark.sql.catalog.session`. 2. add a public abstract class `CatalogExtension` for users to write implementations. TODOs for followup PRs: 1. discuss if we should allow users to completely overwrite the v2 session catalog with a new one. 2. discuss to change the name of session catalog, so that it's less likely to conflict with existing namespace names. ## How was this patch tested? existing tests Closes apache#25104 from cloud-fan/session-catalog. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan commented Jul 11, 2019

View reviewed changes

...ore/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala Outdated Show resolved Hide resolved

cloud-fan commented Jul 11, 2019

View reviewed changes

dongjoon-hyun added the SQL label Jul 11, 2019

rdblue reviewed Jul 11, 2019

View reviewed changes

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

Copy link

Contributor

rdblue Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. No need for get.

brkyvz suggested changes Jul 11, 2019

View reviewed changes

cloud-fan force-pushed the session-catalog branch 2 times, most recently from 6a6b362 to 4f89b88 Compare July 12, 2019 12:52

cloud-fan changed the title ~~[SPARK-28341][SQL] remove session catalog config~~ [SPARK-28341][SQL] refine the v2 session catalog config Jul 12, 2019

cloud-fan force-pushed the session-catalog branch from 4f89b88 to 5f0c5e1 Compare July 12, 2019 13:43

brkyvz reviewed Jul 12, 2019

View reviewed changes

rdblue reviewed Jul 12, 2019

View reviewed changes

rdblue mentioned this pull request Jul 17, 2019

[SPARK-28331][SQL] Catalogs.load() should be able to load built-in catalogs #25094

Closed

cloud-fan force-pushed the session-catalog branch from 5f0c5e1 to c68d63b Compare August 7, 2019 17:01

cloud-fan added 2 commits September 5, 2019 15:53

Merge remote-tracking branch 'origin/master' into session-catalog

75e2ca7

fix tests

f89f25a

fix test

494e031

Merge remote-tracking branch 'origin/master' into session-catalog

8240062

brkyvz reviewed Sep 6, 2019

View reviewed changes

brkyvz approved these changes Sep 6, 2019

View reviewed changes

add error log

05db860

cloud-fan closed this in abec6d7 Sep 9, 2019

[SPARK-28341][SQL] create a public API for V2SessionCatalog #25104

[SPARK-28341][SQL] create a public API for V2SessionCatalog #25104

Uh oh!

Conversation

cloud-fan commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

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!

cloud-fan commented Jul 11, 2019

Uh oh!

SparkQA commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

rdblue commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 12, 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!

SparkQA commented Jul 12, 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 5, 2019

Uh oh!

SparkQA commented Sep 5, 2019

Uh oh!

SparkQA commented Sep 5, 2019

Uh oh!

SparkQA commented Sep 5, 2019

Uh oh!

SparkQA commented Sep 6, 2019

Uh oh!

cloud-fan commented Sep 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 11, 2019 •

edited

Loading

rdblue commented Jul 11, 2019 •

edited

Loading