[SPARK-30497][SQL] migrate DESCRIBE TABLE to the new framework #27187

cloud-fan · 2020-01-13T07:43:03Z

What changes were proposed in this pull request?

Use the new framework to resolve the DESCRIBE TABLE command.

The v1 DESCRIBE TABLE command supports both table and view. Checked with Hive and Presto, they don't have DESCRIBE TABLE syntax but only DESCRIBE, which supports both table and view:

We should make it clear that DESCRIBE support both table and view, by renaming the command to DescribeRelation.

This PR also tunes the framework a little bit to support the case that a command accepts both table and view.

Why are the changes needed?

This is a part of effort to make the relation lookup behavior consistent: SPARK-29900.

Note that I make a separate PR here instead of #26921, as I need to update the framework to support a new use case: accept both table and view.

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing tests

cloud-fan · 2020-01-13T07:48:19Z

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

+ * Holds the name of a namespace that has yet to be looked up in a catalog. It will be resolved to
+ * [[ResolvedNamespace]] during analysis.
+ */
+case class UnresolvedNamespace(multipartIdentifier: Seq[String]) extends LeafNode {


move these new unresolve plans to one file.

nit: v2Unresolved seems not to fit all

how about v2ResolutionPlans?

cloud-fan · 2020-01-13T07:49:03Z

cc @imback82 @yaooqinn @viirya @HyukjinKwon

yaooqinn · 2020-01-13T08:01:11Z

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

+    relation: LogicalPlan,
+    partitionSpec: TablePartitionSpec,
+    isExtended: Boolean) extends Command {
+  override def children: Seq[LogicalPlan] = Seq(relation)


this def is needed for all commands resolved by the new framework, we may create a trait to reduced redundancy

SparkQA · 2020-01-13T08:05:01Z

Test build #116611 has finished for PR 27187 at commit 4e0e8c1.

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

SparkQA · 2020-01-13T08:05:02Z

Test build #116609 has finished for PR 27187 at commit 0b729b9.

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

cloud-fan · 2020-01-13T08:07:56Z

retest this please

SparkQA · 2020-01-13T09:07:09Z

Test build #116614 has finished for PR 27187 at commit 4e0e8c1.

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

SparkQA · 2020-01-13T13:12:51Z

Test build #116627 has finished for PR 27187 at commit 7dcd69b.

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

SparkQA · 2020-01-13T18:52:13Z

Test build #116643 has finished for PR 27187 at commit 234b091.

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

viirya · 2020-01-14T06:20:34Z

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

+            CatalogV2Util.loadTable(catalog, ident) match {
+              // We don't support view in v2 catalog yet. Here we treat v1 view as table and use
+              // `ResolvedTable` to carry it.
+              case Some(table) => ResolvedTable(catalog.asTableCatalog, ident, table)


Do you mean to add a case?

CatalogV2Util.loadTable(catalog, ident) match { case Some(v1Table: V1Table) if v1Table.v1Table.tableType == CatalogTableType.VIEW => ... }

Otherwise I'm not sure why case Some(table) => is for v1 view.

good point. Adding a case is clearer.

viirya · 2020-01-14T06:29:12Z

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

+      case u @ UnresolvedTableOrView(ident) =>
+        lookupTempView(ident).getOrElse(u)


Looks like it is very similar to UnresolvedRelation?

UnresolvedRelation can also be resolve to view by ResolveTempViews, can be resolved to v2 relation by ResolveTables, can be resolved to v1 relation by ResolveRelations.

Seems we have a few similar ways to do resolving?

some commands don't need to scan the table/view (like DESCRIBE). If we use UnresolvedRelation, then we will end up with DataSourceV2ScanRelation, which creates Scan from Table and is not necessary.

Another problem is view. UnresolvedRelation can be resolved to View, and then get removed at the beginning of optimizer.

I'll think of how to overcome these issues and reuse UnresolvedRelation.

I did more investigation, and think we can't reuse UnresolvedRelation.

UnresolvedRelation means something we want to read/write, so it needs to resolve the data source, and fail if something goes wrong, e.g. the data source class can't be loaded.

For DESCRIBE commands, it should work as long as we can get the table. For example, we may want to use DESCRIBE to debug a table which can't load data source class.

cloud-fan · 2020-01-14T13:07:21Z

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

+ */
+// TODO: create a generic representation for temp view, v1 view and v2 view, after we add view
+//       support to v2 catalog. For now we only need the identifier to fallback to v1 command.
+case class ResolvedView(identifier: Identifier) extends LeafNode {


We can add UnresolvedView when we migrate ALTER VIEW command. @imback82

Sounds good. Thanks!

SparkQA · 2020-01-14T13:22:50Z

Test build #116707 has finished for PR 27187 at commit 8ed967c.

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

SparkQA · 2020-01-14T13:31:56Z

Test build #116708 has started for PR 27187 at commit 7654a20.

SparkQA · 2020-01-14T14:19:30Z

Test build #116706 has finished for PR 27187 at commit 7654a20.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ResolvedView(identifier: Identifier) extends LeafNode

SparkQA · 2020-01-14T19:44:33Z

Test build #116715 has finished for PR 27187 at commit 1919054.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ResolvedView(identifier: Identifier) extends LeafNode

imback82

LGTM. (This is a good enhancement to the resolution framework!)

imback82 · 2020-01-15T02:17:42Z

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

+ */
+// TODO: create a generic representation for temp view, v1 view and v2 view, after we add view
+//       support to v2 catalog. For now we only need the identifier to fallback to v1 command.
+case class ResolvedView(identifier: Identifier) extends LeafNode {


Sounds good. Thanks!

imback82 · 2020-01-15T02:54:13Z

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

+              case None => u
+            }
+          case _ => u
+        }


nit: This is almost same as UnresolvedTable case? Maybe we can do the following, or may not be worthwhile:

case u @ UnresolvedTable(identifier) => lookupTableOrView(u, identifier) { u.failAnalysis(s"$ident is a view not table.") } case u @ UnresolvedTableOrView(identifier) => lookupTableOrView(u, identifier) { ResolvedView(identifier) } } private def lookupTableOrView( unresolved: LogicalPlan, identifier: Seq[String])(viewHandler: => LogicalPlan): LogicalPlan = { expandRelationName(identifier) match { case SessionCatalogAndIdentifier(catalog, ident) => CatalogV2Util.loadTable(catalog, ident) match { case Some(v1Table: V1Table) if v1Table.v1Table.tableType == CatalogTableType.VIEW => viewHandler case Some(table) => ResolvedTable(catalog.asTableCatalog, ident, table) case None => unresolved } case _ => unresolved }

imback82 · 2020-01-15T02:58:18Z

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

+    case DescribeRelation(ResolvedTable(_, ident, _: V1Table), partitionSpec, isExtended) =>
+      DescribeTableCommand(ident.asTableIdentifier, partitionSpec, isExtended)
+
+    // Use v1 command to describe temp view, as v2 catalog doesn't support view yet.


describe (temp) view

imback82 · 2020-01-15T02:59:00Z

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

+
+    // Use v1 command to describe temp view, as v2 catalog doesn't support view yet.
+    case DescribeRelation(ResolvedView(ident), partitionSpec, isExtended) =>
+      DescribeTableCommand(ident.asTableIdentifier, partitionSpec, isExtended)


looks clean 👍

HyukjinKwon

LGTM

SparkQA · 2020-01-15T08:05:02Z

Test build #116747 has finished for PR 27187 at commit 5622322.

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

cloud-fan · 2020-01-15T08:06:16Z

retest this please

cloud-fan · 2020-01-15T13:11:01Z

retest this please

SparkQA · 2020-01-15T14:50:50Z

Test build #116763 has finished for PR 27187 at commit 5622322.

This patch fails from timeout after a configured wait of 400m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-15T17:28:11Z

Test build #116784 has finished for PR 27187 at commit 5622322.

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

gatorsmile · 2020-01-16T01:39:00Z

Thanks! Merged to master

gatorsmile · 2020-01-16T01:39:13Z

cc @imback82

imback82 · 2020-01-16T07:50:55Z

Thanks. I will start migrating more commands to the new framework this week.

cloud-fan force-pushed the describe branch from 0b729b9 to 4e0e8c1 Compare January 13, 2020 07:46

cloud-fan commented Jan 13, 2020

View reviewed changes

cloud-fan mentioned this pull request Jan 13, 2020

[SPARK-30282][SQL] Migrate SHOW TBLPROPERTIES to new framework #26921

Closed

yaooqinn reviewed Jan 13, 2020

View reviewed changes

cloud-fan force-pushed the describe branch from 4e0e8c1 to 7dcd69b Compare January 13, 2020 09:22

rename DescribeTable to DescribeRelation and migrate to new framework

234b091

cloud-fan force-pushed the describe branch from 7dcd69b to 234b091 Compare January 13, 2020 14:11

yaooqinn approved these changes Jan 14, 2020

View reviewed changes

viirya reviewed Jan 14, 2020

View reviewed changes

cloud-fan commented Jan 14, 2020

View reviewed changes

cloud-fan force-pushed the describe branch from 8ed967c to 7654a20 Compare January 14, 2020 13:25

address comments

1919054

cloud-fan force-pushed the describe branch from 7654a20 to 1919054 Compare January 14, 2020 15:34

imback82 approved these changes Jan 15, 2020

View reviewed changes

address comments

5622322

HyukjinKwon approved these changes Jan 15, 2020

View reviewed changes

viirya approved these changes Jan 15, 2020

View reviewed changes

gatorsmile closed this in 883ae33 Jan 16, 2020

dongjoon-hyun added the SQL label Feb 5, 2020

		case u @ UnresolvedTableOrView(ident) =>
		lookupTempView(ident).getOrElse(u)

[SPARK-30497][SQL] migrate DESCRIBE TABLE to the new framework #27187

[SPARK-30497][SQL] migrate DESCRIBE TABLE to the new framework #27187

Uh oh!

Conversation

cloud-fan commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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 Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 13, 2020

Uh oh!

SparkQA commented Jan 13, 2020

Uh oh!

cloud-fan commented Jan 13, 2020

Uh oh!

SparkQA commented Jan 13, 2020

Uh oh!

SparkQA commented Jan 13, 2020

Uh oh!

SparkQA commented Jan 13, 2020

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!

SparkQA commented Jan 14, 2020

Uh oh!

SparkQA commented Jan 14, 2020

Uh oh!

SparkQA commented Jan 14, 2020

Uh oh!

SparkQA commented Jan 14, 2020

Uh oh!

imback82 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!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 15, 2020

Uh oh!

cloud-fan commented Jan 15, 2020

Uh oh!

cloud-fan commented Jan 15, 2020

Uh oh!

SparkQA commented Jan 15, 2020

Uh oh!

SparkQA commented Jan 15, 2020

Uh oh!

gatorsmile commented Jan 16, 2020

cloud-fan commented Jan 13, 2020 •

edited

Loading

imback82 commented Jan 16, 2020 •

edited

Loading