[SPARK-28856][SQL] Implement SHOW DATABASES for Data Source V2 Tables #25601

imback82 · 2019-08-27T23:20:38Z

What changes were proposed in this pull request?

Implement the SHOW DATABASES logical and physical plans for data source v2 tables.

Why are the changes needed?

To support SHOW DATABASES SQL commands for v2 tables.

Does this PR introduce any user-facing change?

spark.sql("SHOW DATABASES") will return namespaces if the default catalog is set:

+---------------+
|      namespace|
+---------------+
|            ns1|
|      ns1.ns1_1|
|ns1.ns1_1.ns1_2|
+---------------+

How was this patch tested?

Added unit tests to DataSourceV2SQLSuite.

imback82 · 2019-08-27T23:20:55Z

cc: @rdblue @cloud-fan

SparkQA · 2019-08-28T03:08:35Z

Test build #109836 has finished for PR 25601 at commit 87fe6ec.

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

cloud-fan · 2019-08-28T04:34:53Z

shall we call it SHOW NAMESPACES?

imback82 · 2019-08-29T01:16:22Z

Do we need to support both SHOW DATABASES and SHOW NAMESPACES or just SHOW NAMESPACES?

cloud-fan · 2019-08-29T13:00:32Z

I think we have to keep SHOW DATABASES for backward compatibility. We can just treat SHOW DATABASES as an alias of SHOW NAMESPACES. @rdblue what do you think?

rdblue · 2019-08-29T23:59:11Z

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

+    pattern: Option[String])
+    extends LeafExecNode {
+  override protected def doExecute(): RDD[InternalRow] = {
+    val namespaces = catalog.listNamespaces().flatMap(getNamespaces(catalog, _))


This shouldn't list the entire space. It should only call listNamespaces once. If the current namespace is and empty array then call listNamespaces() and if it is anything else, call listNamespaces(current).

From the SPIP:

For a given operation, Spark will call the corresponding catalog method once. For example, SHOW TABLES will return results from listTables(currentNamespace). Spark will not traverse nested namespaces with multiple calls to listNamespaces and listTables.

If the current namespace is and empty array then call listNamespaces()

I just realized that this isn't the same behavior as v1. In v1, SHOW DATABASES ignores the current database because databases aren't nested. It always lists all databases (then filters).

The proposed behavior of SHOW NAMESPACES was to respect the current namespace and list namespaces nested in it.

There are a few options to fix this:

Add SHOW NAMESPACES that behaves differently than SHOW DATABASES

Make SHOW NAMESPACES list all namespaces recursively, like this PR

Make SHOW NAMESPACES list the namespace above the current. If current=a.b, then list a and show the results (including b).

Change the behavior of SHOW DATABASES to match SHOW NAMESPACES and list the current

Change the behavior of SHOW DATABASES to match SHOW NAMESPACES and list the current, but match behavior if the current namespace is "default"

@imback82, @brkyvz, @cloud-fan, @mccheah, any opinion here?

Add SHOW NAMESPACES that behaves differently than SHOW DATABASES

I prefer this.

Another idea is: SHOW NAMESPACES should list the root namespaces of the current catalog, no matter what the current namespace is.

Another option is to ignore the current namespace entirely. SHOW NAMESPACES would list the root, and SHOW NAMESPACES IN ns1 lists namespaces in ns1. The context is always explicit.

I think I would prefer that option.

Another option is to ignore the current namespace entirely. SHOW NAMESPACES would list the root, and SHOW NAMESPACES IN ns1 lists namespaces in ns1. The context is always explicit.

I like this idea. @cloud-fan are you OK with this approach?

Yea that's exactly what I mentioned before, with addition of SHOW NAMESPACES IN ns1, +1

Another idea is: SHOW NAMESPACES should list the root namespaces of the current catalog, no matter what the current namespace is.

@cloud-fan @rdblue Thanks for your suggestions.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

imback82 · 2019-09-04T02:44:57Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

    | DROP database (IF EXISTS)? db=errorCapturingIdentifier
        (RESTRICT | CASCADE)?                                          #dropDatabase
    | SHOW DATABASES (LIKE? pattern=STRING)?                           #showDatabases
+    | SHOW NAMESPACES ((FROM | IN) multipartIdentifier)?


I put both FROM and IN similar to SHOW TABLES. Please let me know if FROM is not needed.

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

sql/catalyst/src/test/scala/org/apache/spark/sql/connector/InMemoryTableCatalog.scala

imback82 · 2019-09-04T02:50:25Z

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

+    extends LeafExecNode {
+  override protected def doExecute(): RDD[InternalRow] = {
+    val namespaces = namespace.map{ ns =>
+        if (ns.nonEmpty) {


@rdblue @cloud-fan this is for handling the case SHOW NAMESPACES IN catalogname. In this case, should we list the root namespaces or call listNamespaces with an empty array?

should we list the root namespaces or call listNamespaces with an empty array?

I think these 2 are the same?

From the SPIP, I see the following:

SHOW NAMESPACES IN foo Returns the result of sparkSession.catalog("foo").listNamespaces().

Since the behavior of listNamespaces(Array()) depends on the implementation, I think it's safe to check and call listNamespaces(). @rdblue What do you think?

Calling listNamespaces() sounds good to me.

SparkQA · 2019-09-04T04:17:02Z

Test build #110080 has finished for PR 25601 at commit 9974a58.

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

SparkQA · 2019-09-04T04:26:14Z

Test build #110081 has finished for PR 25601 at commit ba1e7f4.

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

…w_databases

SparkQA · 2019-09-04T04:35:34Z

Test build #110083 has finished for PR 25601 at commit 9f738cf.

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

cloud-fan · 2019-09-04T05:36:01Z

docs/sql-keywords.md

  <tr><td>MONTH</td><td>reserved</td><td>non-reserved</td><td>reserved</td></tr>
  <tr><td>MONTHS</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
  <tr><td>MSCK</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>
+  <tr><td>NAMESPACES</td><td>non-reserved</td><td>non-reserved</td><td>non-reserved</td></tr>


cc @xianyinxin , we should also add DELETE and UPDATE. Can you open a PR to do it?

ok, will open a pr.

DELETE is already there. UPDATE is included in #25626

cloud-fan · 2019-09-04T05:55:35Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala

      ShowTablesStatement(Some(Seq("tbl")), Some("*dog*")))
  }

+  test("show namespaces") {


cc @xianyinxin can you add similar parser tests for DELETE/UPDATE as well?

For update, #25626 has added some parser cases. For delete, will done in #25652.

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

Test build #110093 has finished for PR 25601 at commit 672f526.

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

cloud-fan · 2019-09-04T07:25:06Z

retest this please

SparkQA · 2019-09-04T11:08:03Z

Test build #110105 has finished for PR 25601 at commit 672f526.

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

cloud-fan · 2019-09-04T12:54:21Z

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

+      val CatalogNamespace(maybeCatalog, ns) = namespace
+      maybeCatalog match {
+        case Some(catalog: SupportsNamespaces) =>
+          ShowNamespaces(catalog, Some(ns), pattern)


maybe better to write if (ns.nonEmpty) Some(ns) else None

I put this logic in ShowNamespacesExec. If we move here, there is an implicit contract that if namespace is Some, it should be nonEmpty (meaning I need to add a require check to make it explicit in ShowNamespaceExec).

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

cloud-fan · 2019-09-04T13:00:37Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+ */
+case class ShowNamespaces(
+    catalog: SupportsNamespaces,
+    namespace: Option[Seq[String]],


After reading the code, it's actually catalogAndNamespace, right?

This is just namespace since catalog is already resolved in DataSourceResolution.scala.

cloud-fan · 2019-09-04T13:00:53Z

...src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/ShowNamespacesStatement.scala

+/**
+ * A SHOW NAMESPACES statement, as parsed from SQL.
+ */
+case class ShowNamespacesStatement(namespace: Option[Seq[String]], pattern: Option[String])


Yes, this is catalog + namespace, but I followed the same convention as other statements - i.e., CreateTableStatement has tableName instead of catalogAndTableName. Please let me know if you prefer catalogAndNamespace here.

cloud-fan · 2019-09-04T13:05:18Z

LGTM except some code style issues.

SparkQA · 2019-09-04T21:01:38Z

Test build #110137 has finished for PR 25601 at commit 6256aeb.

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

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

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

rdblue · 2019-09-04T21:57:05Z

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

+        case Some(catalog: SupportsNamespaces) =>
+          ShowNamespaces(catalog, Some(ns), pattern)
+        case _ =>
+          throw new AnalysisException(


I think this needs to distinguish between the case where the catalog is None and the catalog does not support namespaces. For the second case, this should report that the catalog doesn't support namespaces. You can also add a conversion method, asNamespaceCatalog to CatalogV2Utils like asTableCatalog.

Using asNamespaceCatalog simplifies the matching. Thanks.

Why not use the current catalog instead of failing?

If the catalog name is specified, but catalog doesn't support namespace, I think we should fail instead of falling back to the current catalog.

It's similar to: if the catalog name is specified, but doesn't contain the table we need, we should fail instead of falling back to the current catalog.

SparkQA · 2019-09-05T00:14:20Z

Test build #110155 has finished for PR 25601 at commit 2295f91.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

rdblue · 2019-09-06T21:38:12Z

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

      DeleteFromTable(aliased, delete.condition)

+    case ShowNamespacesStatement(None, pattern) =>
+      defaultCatalog match {


I think this should be currentCatalog instead. @cloud-fan, do you agree?

Let's implement switching the current catalog first, otherwise we are not able to test it.

@imback82 are you working on it?

yes, I am working on USE NAMESPACE

I should be able to send out the PR sometime tomorrow.

rdblue · 2019-09-06T21:48:14Z

This looks good to me other than the behavior when the catalog is not included in the IN clause. This uses defaultCatalog, but now the way we track catalogs has changed a bit:

The current catalog should be used when no catalog is specified
The default catalog is the catalog current is initialized to
If the default catalog is not set, then it is the built-in Spark session catalog, which I think we intend to call spark_catalog

It looks like these rules are not universally followed, so I've opened SPARK-29014 to track this clean-up. I'm okay merging this and fixing the catalog defaults in a follow-up.

FYI: @cloud-fan, @brkyvz.

imback82 · 2019-09-10T01:49:57Z

@cloud-fan do you have any other comments other than what @rdblue brought up?

SparkQA · 2019-09-10T03:21:55Z

Test build #110378 has finished for PR 25601 at commit 9a55a03.

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

cloud-fan · 2019-09-10T03:29:51Z

retest this please

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

Test build #110393 has finished for PR 25601 at commit 9a55a03.

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

cloud-fan · 2019-09-10T09:14:58Z

retest this please

SparkQA · 2019-09-10T12:48:01Z

Test build #110411 has finished for PR 25601 at commit 9a55a03.

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

cloud-fan · 2019-09-10T13:24:09Z

thanks, merging to master!

### What changes were proposed in this pull request? Implement the SHOW DATABASES logical and physical plans for data source v2 tables. ### Why are the changes needed? To support `SHOW DATABASES` SQL commands for v2 tables. ### Does this PR introduce any user-facing change? `spark.sql("SHOW DATABASES")` will return namespaces if the default catalog is set: ``` +---------------+ | namespace| +---------------+ | ns1| | ns1.ns1_1| |ns1.ns1_1.ns1_2| +---------------+ ``` ### How was this patch tested? Added unit tests to `DataSourceV2SQLSuite`. Closes apache#25601 from imback82/show_databases. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

imback82 added 3 commits August 27, 2019 15:49

initial checkin

1f3b8b9

Merge branch 'master' into show_databases

bab1a8f

update tests

87fe6ec

dongjoon-hyun added the SQL label Aug 28, 2019

rdblue reviewed Aug 29, 2019

View reviewed changes

rdblue reviewed Aug 30, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

imback82 added 2 commits September 3, 2019 16:40

Merge branch 'master' into show_databases

da65365

Introduce SHOW NAMESPACES.

9974a58

imback82 commented Sep 4, 2019

View reviewed changes

imback82 added 2 commits September 3, 2019 19:51

Fix comments.

ba1e7f4

Update sql-keywords.md

9f738cf

imback82 added 2 commits September 3, 2019 21:27

fix tests

7d606e1

Merge branch 'show_databases' of github.com:imback82/spark-4 into sho…

672f526

…w_databases

cloud-fan reviewed Sep 4, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Sep 4, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Sep 4, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Sep 4, 2019

View reviewed changes

Address comments

6256aeb

rdblue reviewed Sep 4, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowNamespacesExec.scala Outdated Show resolved Hide resolved

rdblue reviewed Sep 4, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowNamespacesExec.scala Outdated Show resolved Hide resolved

rdblue reviewed Sep 4, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala Outdated Show resolved Hide resolved

rdblue reviewed Sep 4, 2019

View reviewed changes

Address PR comments

2295f91

rdblue reviewed Sep 6, 2019

View reviewed changes

Merge branch 'master' into show_databases

9a55a03

cloud-fan approved these changes Sep 10, 2019

View reviewed changes

cloud-fan closed this in bf43541 Sep 10, 2019

imback82 mentioned this pull request Sep 10, 2019

[SPARK-29039][SQL] centralize the catalog and table lookup logic #25747

Closed

[SPARK-28856][SQL] Implement SHOW DATABASES for Data Source V2 Tables #25601

[SPARK-28856][SQL] Implement SHOW DATABASES for Data Source V2 Tables #25601

Uh oh!

Conversation

imback82 commented Aug 27, 2019

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?

Uh oh!

imback82 commented Aug 27, 2019

Uh oh!

SparkQA commented Aug 28, 2019

Uh oh!

cloud-fan commented Aug 28, 2019

Uh oh!

imback82 commented Aug 29, 2019

Uh oh!

cloud-fan commented Aug 29, 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!

cloud-fan 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 Sep 4, 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!

SparkQA commented Sep 4, 2019

Uh oh!

SparkQA commented Sep 4, 2019

Uh oh!

SparkQA commented Sep 4, 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 4, 2019

Uh oh!

cloud-fan commented Sep 4, 2019

Uh oh!

SparkQA commented Sep 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan Sep 2, 2019 •

edited

Loading

rdblue Sep 4, 2019 •

edited

Loading

imback82 Sep 4, 2019 •

edited

Loading