[SPARK-29519][SQL] SHOW TBLPROPERTIES should do multi-catalog resolution. #26176

planga82 · 2019-10-19T16:42:58Z

What changes were proposed in this pull request?

Add ShowTablePropertiesStatement and make SHOW TBLPROPERTIES go through the same catalog/table resolution framework of v2 commands.

Why are the changes needed?

It's important to make all the commands have the same table resolution behavior, to avoid confusing end-users. e.g.

USE my_catalog
DESC t // success and describe the table t from my_catalog
SHOW TBLPROPERTIES t // report table not found as there is no table t in the session catalog

Does this PR introduce any user-facing change?

yes. When running SHOW TBLPROPERTIES Spark fails the command if the current catalog is set to a v2 catalog, or the table name specified a v2 catalog.

How was this patch tested?

Unit tests.

planga82 · 2019-10-19T16:47:03Z

cc: @cloud-fan @rdblue @viirya @imback82

viirya · 2019-10-19T17:46:07Z

ok to test

SparkQA · 2019-10-19T20:27:37Z

Test build #112320 has finished for PR 26176 at commit 4bd0c5a.

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

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

Test build #112335 has finished for PR 26176 at commit 4bc65b1.

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

SparkQA · 2019-10-20T10:59:32Z

Test build #112338 has finished for PR 26176 at commit f651227.

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

imback82 · 2019-10-20T14:00:21Z

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

+  override def visitShowTblProperties(
+      ctx: ShowTblPropertiesContext): LogicalPlan = withOrigin(ctx) {
+    ShowTablePropertiesStatement(
+      visitMultipartIdentifier(ctx.multipartIdentifier()),


ctx.table since the alias is available? I am not sure what the preferred way is, and for commands like ALTER TABLE, we haven't used the alias table in SqlBase.g4. @cloud-fan can confirm.

I have seen the other pull requests as example. If we have to change it, it's not a problem.

It's not a big deal. We can either remove the table alias, or use ctx.table here.

cloud-fan · 2019-10-21T03:39:05Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+      val e1 = intercept[AnalysisException] {
+        sql(s"SHOW TBLPROPERTIES $t")
+      }
+      assert(e1.message.contains("SHOW TBLPROPERTIES is only supported with v1 tables"))


I think we can implement SHOW TBLPROPERTIES with v2 APIs. You can refer to #26166 about how to implement a v2 command.

planga82 · 2019-11-06T05:54:11Z

V2 command implemented

cloud-fan · 2019-11-06T06:46:05Z

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

+/**
+ * The logical plan of the SHOW TBLPROPERTIES command that works for v2 catalogs.
+ */
+case class ShowTblproperties(


let's use the full name: ShowTableProperties

cloud-fan · 2019-11-06T06:48:03Z

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

      ShowCurrentNamespace(catalogManager)
+
+    case ShowTablePropertiesStatement(NonSessionCatalog(catalog, tableName), propertyKey) =>
+      ShowTblproperties(catalog.asTableCatalog, tableName.asIdentifier, propertyKey)


The logical plan should take a NamedRelation, and here we can just pass in a UnresolvedV2Relation.

You can take a look at how DescribeTableStatement is handled in this file.

Ok, I see, the Analizer resolve tables in V2 catalog, thanks!!

SparkQA · 2019-11-06T08:05:02Z

Test build #113301 has finished for PR 26176 at commit fb93e0f.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ShowTablePropertiesStatement(
case class ShowTblproperties(
case class ShowTblpropertiesExec(

SparkQA · 2019-11-07T22:39:12Z

Test build #113400 has finished for PR 26176 at commit 706a555.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ShowTableProperties(

cloud-fan · 2019-11-08T01:07:37Z

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

      ShowCurrentNamespace(catalogManager)
+
+    case ShowTablePropertiesStatement(
+    nameParts @ NonSessionCatalog(catalog, tableName), propertyKey) =>


nit: 4 space indentation before nameParts @

cloud-fan · 2019-11-08T01:08:36Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+      spark.sql(s"CREATE TABLE $t (id bigint, data string) USING $provider " +
+        s"TBLPROPERTIES ('owner'='$owner', 'status'='$status')")
+
+      val tbl1 = sql(s"SHOW TBLPROPERTIES $t ('status')")


can we also test if the key not found?

imback82 · 2019-11-08T16:08:54Z

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

+      nameParts @ NonSessionCatalog(catalog, tableName), propertyKey) =>
+        val r = UnresolvedV2Relation(nameParts, catalog.asTableCatalog, tableName.asIdentifier)
+        ShowTableProperties(r, propertyKey)
+


nit: remove extra empty line.

imback82 · 2019-11-08T16:10:00Z

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

+   * corresponding values are returned.
+   * The syntax of using this command in SQL is:
+   * {{{
+   *   SHOW TBLPROPERTIES table_name[('propertyKey')];


nit: table_name -> multi_part_name

imback82 · 2019-11-08T16:12:16Z

...ore/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowTblpropertiesExec.scala

+          encoder.toRow(new GenericRowWithSchema(Array(k, properties(k)), schema)).copy()).toSeq
+    }
+  }
+


nit: remove empty line

imback82 · 2019-11-08T16:13:08Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+
+      assert(tbl1.schema === schema)
+      assert(expected === tbl1.collect())
+


nit: remove empty line

imback82 · 2019-11-08T16:16:32Z

...ore/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowTblpropertiesExec.scala

+/**
+ * Physical plan node for showing tblproperties.
+ */
+case class ShowTblpropertiesExec(


ShowTablePropertiesExec to match with the statement, etc. Same applies to other places below.

imback82 · 2019-11-08T16:21:09Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+      spark.sql(s"CREATE TABLE $t (id bigint, data string) USING $provider " +
+        s"TBLPROPERTIES ('owner'='$owner', 'status'='$status')")
+
+      val tbl1 = sql(s"SHOW TBLPROPERTIES $t")


Why 1? How about just properties?

Changes done

SparkQA · 2019-11-08T19:17:04Z

Test build #113466 has finished for PR 26176 at commit 93ec3f8.

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

SparkQA · 2019-11-08T23:06:42Z

Test build #113471 has finished for PR 26176 at commit e5e44c1.

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

imback82

LGTM. Thanks @planga82!

planga82 · 2019-11-09T14:57:27Z

Thanks for your comments!!

[SPARK-29519] Fix tests [SPARK-29519] Resolve catalog in Analizer [SPARK-29519] Fix pull request comments [SPARK-29519] Fix pull request comments

planga82 · 2019-11-10T18:56:08Z

Conflict resolved

SparkQA · 2019-11-10T22:37:48Z

Test build #113547 has finished for PR 26176 at commit 863ed90.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ShowTablePropertiesStatement(
case class ShowTableProperties(
case class ShowTablePropertiesExec(

fuwhu · 2019-11-11T03:01:54Z

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

+        CatalogV2Util.loadRelation(u.catalog, u.tableName)
+          .map(rel => show.copy(table = rel))
+          .getOrElse(u)
+


Why is it '.getOrElse(u)' instead of '.getOrElse(show)' here ?

good catch!

oh, yes good catch!!

cloud-fan · 2019-11-11T04:01:33Z

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

      ShowCurrentNamespace(catalogManager)
+
+    case ShowTablePropertiesStatement(
+      nameParts @ NonSessionCatalog(catalog, tableName), propertyKey) =>


nit: 4 space indentation

SparkQA · 2019-11-11T23:58:13Z

Test build #113599 has finished for PR 26176 at commit 846e21a.

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

cloud-fan · 2019-11-12T05:31:45Z

thanks, merging to master!

dongjoon-hyun added the SQL label Oct 19, 2019

planga82 force-pushed the feature/SPARK-29519_SHOW_TBLPROPERTIES_datasourceV2 branch from 4bc65b1 to f651227 Compare October 20, 2019 07:18

imback82 reviewed Oct 20, 2019

View reviewed changes

cloud-fan reviewed Oct 21, 2019

View reviewed changes

planga82 force-pushed the feature/SPARK-29519_SHOW_TBLPROPERTIES_datasourceV2 branch from f651227 to fb93e0f Compare November 6, 2019 05:51

cloud-fan reviewed Nov 6, 2019

View reviewed changes

cloud-fan reviewed Nov 8, 2019

View reviewed changes

cloud-fan approved these changes Nov 8, 2019

View reviewed changes

imback82 reviewed Nov 8, 2019

View reviewed changes

imback82 approved these changes Nov 9, 2019

View reviewed changes

[SPARK-29519] SHOW TBLPROPERTIES should do multi-catalog resolution.

863ed90

[SPARK-29519] Fix tests [SPARK-29519] Resolve catalog in Analizer [SPARK-29519] Fix pull request comments [SPARK-29519] Fix pull request comments

planga82 force-pushed the feature/SPARK-29519_SHOW_TBLPROPERTIES_datasourceV2 branch from e5e44c1 to 863ed90 Compare November 10, 2019 18:55

fuwhu reviewed Nov 11, 2019

View reviewed changes

cloud-fan reviewed Nov 11, 2019

View reviewed changes

[SPARK-29519] Fix analizer

846e21a

cloud-fan closed this in 37e387a Nov 12, 2019


		assert(tbl1.schema === schema)
		assert(expected === tbl1.collect())

[SPARK-29519][SQL] SHOW TBLPROPERTIES should do multi-catalog resolution. #26176

[SPARK-29519][SQL] SHOW TBLPROPERTIES should do multi-catalog resolution. #26176

Uh oh!

Conversation

planga82 commented Oct 19, 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!

planga82 commented Oct 19, 2019

Uh oh!

viirya commented Oct 19, 2019

Uh oh!

SparkQA commented Oct 19, 2019

Uh oh!

SparkQA commented Oct 20, 2019

Uh oh!

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

planga82 commented Nov 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 6, 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!

SparkQA commented Nov 6, 2019

Uh oh!

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

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 Nov 8, 2019

Uh oh!

SparkQA commented Nov 8, 2019

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

planga82 commented Nov 9, 2019

Uh oh!

planga82 commented Nov 10, 2019

Uh oh!

SparkQA commented Nov 10, 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

cloud-fan Nov 6, 2019 •

edited

Loading