[SPARK-29279][SQL] Merge SHOW NAMESPACES and SHOW DATABASES code path #26006

imback82 · 2019-10-02T23:45:34Z

What changes were proposed in this pull request?

Currently, SHOW NAMESPACES and SHOW DATABASES are separate code paths. This PR merges two implementations.

Why are the changes needed?

To remove code/behavior duplication

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added new unit tests.

imback82 · 2019-10-02T23:45:47Z

cc: @rdblue / @cloud-fan

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

SparkQA · 2019-10-03T02:30:51Z

Test build #111705 has finished for PR 26006 at commit db8968a.

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

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

Test build #111712 has finished for PR 26006 at commit 67114a7.

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

HyukjinKwon · 2019-10-04T08:14:45Z

retest this please

SparkQA · 2019-10-04T12:04:05Z

Test build #111771 has finished for PR 26006 at commit 67114a7.

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

… logic

imback82 · 2019-10-04T17:40:25Z

@cloud-fan I merged with #25747 and is ready for review.

imback82 · 2019-10-04T17:41:30Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

-      throw new AnalysisException(
-        "SHOW NAMESPACES is not supported with the session catalog.")
-
-    // TODO (SPARK-29014): we should check if the current catalog is session catalog here.


I can also handle the ShowTables case below as a separate PR (I have tests written up) if you want.

SparkQA · 2019-10-04T17:42:05Z

Test build #111784 has started for PR 26006 at commit 51966bc.

rdblue · 2019-10-04T19:42:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala

        val CurrentCatalogAndNamespace(catalog, namespace) = nameParts
-        val ns = if (namespace.isEmpty) { None } else { Some(namespace) }
-        SetCatalogAndNamespace(catalogManager, Some(catalog.name()), ns)
+        SetCatalogAndNamespace(catalogManager, Some(catalog.name()), namespace)


This will always set the catalog, even if it is already the current? Why not use a rule that doesn't fill in the catalog and returns None instead?

The check is done here:

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

Lines 128 to 137 in 8556710

def setCurrentCatalog(catalogName: String): Unit = synchronized {

// `setCurrentCatalog` is noop if it doesn't switch to a different catalog.

if (currentCatalog.name() != catalogName) {

_currentCatalogName = Some(catalogName)

_currentNamespace = None

// Reset the current database of v1 `SessionCatalog` when switching current catalog, so that

// when we switch back to session catalog, the current namespace definitely is ["default"].

v1SessionCatalog.setCurrentDatabase(SessionCatalog.DEFAULT_DATABASE)

}

}

rdblue · 2019-10-04T19:44:31Z

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

  }

-  type CurrentCatalogAndNamespace = (CatalogPlugin, Seq[String])
+  type CurrentCatalogAndNamespace = (CatalogPlugin, Option[Seq[String]])


When I see this extractor/type name, it sounds like it should always return the current catalog. What about just naming this CatalogAndNamespace and document that the current catalog may be returned?

Thanks, I will update it.

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

rdblue · 2019-10-04T20:04:57Z

Overall looks good to me. The EXPLAIN change is a little concerning and I had some other minor comments.

cloud-fan · 2019-10-09T06:15:07Z

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

-  }
-
-  type CurrentCatalogAndNamespace = (CatalogPlugin, Seq[String])
+  type CatalogAndNamespace = (CatalogPlugin, Option[Seq[String]])


This is short, maybe we don't need the type alias.

cloud-fan · 2019-10-09T06:18:27Z

LGTM except one code style nit. We may need to wait for a few days until Jenkins is back online.

rdblue · 2019-10-10T21:48:19Z

+1 from me as well

cloud-fan · 2019-10-14T10:25:48Z

retest this please

SparkQA · 2019-10-14T14:07:17Z

Test build #112013 has finished for PR 26006 at commit d8b5362.

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

cloud-fan · 2019-10-14T14:35:58Z

thanks, merging to master!

cloud-fan · 2019-10-14T14:37:30Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

-      throw new AnalysisException(
-        "SHOW NAMESPACES is not supported with the session catalog.")
-
-    // TODO (SPARK-29014): we should check if the current catalog is session catalog here.


@imback82 can you send a PR to fix SPARK-29014? It should be easy now.

Yes, I will work on this. Thanks!

…f SHOW DATABASES ### What changes were proposed in this pull request? This is a followup of #26006 In #26006 , we merged the v1 and v2 SHOW DATABASES/NAMESPACES commands, but we missed a behavior change that the output schema of SHOW DATABASES becomes different. This PR adds a legacy config to restore the old schema, with a migration guide item to mention this behavior change. ### Why are the changes needed? Improve backward compatibility ### Does this PR introduce _any_ user-facing change? No (the legacy config is false by default) ### How was this patch tested? a new test Closes #31474 from cloud-fan/command-schema. Lead-authored-by: Wenchen Fan <cloud0fan@gmail.com> Co-authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…f SHOW DATABASES This is a followup of apache#26006 In apache#26006 , we merged the v1 and v2 SHOW DATABASES/NAMESPACES commands, but we missed a behavior change that the output schema of SHOW DATABASES becomes different. This PR adds a legacy config to restore the old schema, with a migration guide item to mention this behavior change. Improve backward compatibility No (the legacy config is false by default) a new test Closes apache#31474 from cloud-fan/command-schema. Lead-authored-by: Wenchen Fan <cloud0fan@gmail.com> Co-authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ema of SHOW DATABASES This backports #31474 to 3.1/3.0 ### What changes were proposed in this pull request? This is a followup of #26006 In #26006 , we merged the v1 and v2 SHOW DATABASES/NAMESPACES commands, but we missed a behavior change that the output schema of SHOW DATABASES becomes different. This PR adds a legacy config to restore the old schema, with a migration guide item to mention this behavior change. ### Why are the changes needed? Improve backward compatibility ### Does this PR introduce _any_ user-facing change? No (the legacy config is false by default) ### How was this patch tested? a new test Closes #31486 from cloud-fan/command-schema. Authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ema of SHOW DATABASES This backports #31474 to 3.1/3.0 This is a followup of #26006 In #26006 , we merged the v1 and v2 SHOW DATABASES/NAMESPACES commands, but we missed a behavior change that the output schema of SHOW DATABASES becomes different. This PR adds a legacy config to restore the old schema, with a migration guide item to mention this behavior change. Improve backward compatibility No (the legacy config is false by default) a new test Closes #31486 from cloud-fan/command-schema. Authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 7c87b48) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

initial checkin

db8968a

imback82 commented Oct 3, 2019

View reviewed changes

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

Fix unit tests

67114a7

dongjoon-hyun added the SQL label Oct 3, 2019

imback82 added 2 commits October 4, 2019 10:03

Merge branch 'master' into combine_show

fece0a0

Merge with [SPARK-29039][SQL] centralize the catalog and table lookup…

51966bc

… logic

imback82 commented Oct 4, 2019

View reviewed changes

rdblue reviewed Oct 4, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala Outdated Show resolved Hide resolved

imback82 added 2 commits October 8, 2019 21:11

Merge branch 'master' into combine_show

4a3d6a4

Address PR comments

94411cb

cloud-fan reviewed Oct 9, 2019

View reviewed changes

Address PR comments

d8b5362

cloud-fan closed this in ef6dce2 Oct 14, 2019

cloud-fan reviewed Oct 14, 2019

View reviewed changes

imback82 deleted the combine_show branch October 14, 2019 14:54

imback82 mentioned this pull request Jan 30, 2021

[SPARK-30420][SQL][FOLLOWUP] Remove statement logical plans for namespace commands #27125

Closed

cloud-fan mentioned this pull request Feb 4, 2021

[SPARK-34359][SQL] Add a legacy config to restore the output schema of SHOW DATABASES #31474

Closed

cloud-fan mentioned this pull request Feb 5, 2021

[SPARK-34359][SQL][3.1] Add a legacy config to restore the output schema of SHOW DATABASES #31486

Closed

	def setCurrentCatalog(catalogName: String): Unit = synchronized {
	// `setCurrentCatalog` is noop if it doesn't switch to a different catalog.
	if (currentCatalog.name() != catalogName) {
	_currentCatalogName = Some(catalogName)
	_currentNamespace = None
	// Reset the current database of v1 `SessionCatalog` when switching current catalog, so that
	// when we switch back to session catalog, the current namespace definitely is ["default"].
	v1SessionCatalog.setCurrentDatabase(SessionCatalog.DEFAULT_DATABASE)
	}
	}

[SPARK-29279][SQL] Merge SHOW NAMESPACES and SHOW DATABASES code path #26006

[SPARK-29279][SQL] Merge SHOW NAMESPACES and SHOW DATABASES code path #26006

Uh oh!

Conversation

imback82 commented Oct 2, 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 Oct 2, 2019

Uh oh!

Uh oh!

SparkQA commented Oct 3, 2019

Uh oh!

SparkQA commented Oct 3, 2019

Uh oh!

HyukjinKwon commented Oct 4, 2019

Uh oh!

SparkQA commented Oct 4, 2019

Uh oh!

imback82 commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 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!

Uh oh!

rdblue commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Oct 9, 2019

Uh oh!

rdblue commented Oct 10, 2019

Uh oh!

cloud-fan commented Oct 14, 2019

Uh oh!

SparkQA commented Oct 14, 2019

Uh oh!

cloud-fan commented Oct 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants