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

imback82 · 2019-12-17T07:10:16Z

What changes were proposed in this pull request?

Use the new framework to resolve the SHOW TBLPROPERTIES command. This PR along with #27243 should update all the existing V2 commands with UnresolvedV2Relation.

Why are the changes needed?

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

Does this PR introduce any user-facing change?

Yes SHOW TBLPROPERTIES temp_view now fails with AnalysisException will be thrown with a message temp_view is a temp view not table. Previously, it was returning empty row.

How was this patch tested?

Existing tests

SparkQA · 2019-12-17T08:05:01Z

Test build #115436 has finished for PR 26921 at commit 5d8422d.

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

SparkQA · 2019-12-17T08:05:02Z

Test build #115437 has finished for PR 26921 at commit db311fd.

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

SparkQA · 2019-12-17T10:05:16Z

Test build #115447 has finished for PR 26921 at commit ae524c3.

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

SparkQA · 2019-12-17T11:16:25Z

Test build #115445 has finished for PR 26921 at commit fc2c5a3.

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

SparkQA · 2019-12-17T21:32:57Z

Test build #115470 has finished for PR 26921 at commit 467145f.

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

imback82 · 2019-12-18T06:02:40Z

cc: @cloud-fan

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

cloud-fan · 2019-12-19T14:42:03Z

I have some ideas about how to move this forward, @imback82 can you take a look at #26847 (comment) and share your thoughts?

imback82

@cloud-fan I integrated two commands that were using UnresolvedV2Relation (ShowTableProperties and DescribeTable). Please let me know if the approach looks fine, then I will finish up AlterTable (and move onto other commands). Thanks!

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

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

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala

SparkQA · 2020-01-09T04:57:51Z

Test build #116343 has finished for PR 26921 at commit a845488.

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

SparkQA · 2020-01-09T05:10:56Z

Test build #116341 has finished for PR 26921 at commit 517793f.

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

cloud-fan · 2020-01-09T05:38:36Z

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

-        } else {
-          if (partitionSpec.nonEmpty) {
-            throw new AnalysisException("DESCRIBE TABLE does not support partition for v2 tables.")
+    case d @ DescribeTable(SessionCatalogAndResolvedTable(resolved), partitionSpec, isExtended) =>


seems simpler to write

d @ DescribeTable(r: ResolvedTable, partitionSpec, isExtended) if isSessionCatalog(r.catalog) =>

cloud-fan · 2020-01-09T05:49:45Z

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

+          DescribeTableCommand(getTableIdentifier(resolved), partitionSpec, isExtended)
+        case _ =>
+          // The v1 `DescribeTableCommand` can describe view as well.
+          if (isView(resolved.identifier.asMultipartIdentifier)) {


before we reach here, we may already fail with table/view mismatch.

Actually, I checked with hive: https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-DescribeTable/View/MaterializedView/Column

The DESCRIBE command works for both table and view, in Spark we have a weird parser rule DESC TABLE? which makes people think it's for table only.

We can keep the parser rule, but we should rename DescribeTable to DescribeRelation, and use UnresolvedRelation to allow both table and view.

same for presto: https://prestodb.io/docs/current/sql/describe.html

I've open #27187 to refine it.

This is a feature we missed when designing the new framework, so I opened a separated PR to update the framework to support accepting both table and view like DESCRIBE command.

imback82 · 2020-01-10T16:45:52Z

Based on the discussion from #26847 (comment), should I make this PR WIP and try to integrate all commands first to verify the new framework is solid? Afterwards, we can split this PR into multiple ones.

cloud-fan · 2020-01-12T07:19:51Z

SGTM

### 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: 1. https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-DescribeTable/View/MaterializedView/Column 2. https://prestodb.io/docs/current/sql/describe.html 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 Closes #27187 from cloud-fan/describe. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Xiao Li <gatorsmile@gmail.com>

imback82 · 2020-01-17T00:05:41Z

@cloud-fan, actually, it was a big change to migrate all commands into one WIP PR (also prob. hard to review anyway). I just changed this PR to only migrate SHOW TBLPROPERTIES. Hope this is OK.

SparkQA · 2020-01-17T02:16:30Z

Test build #116877 has finished for PR 26921 at commit 4fe3e69.

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

SparkQA · 2020-01-17T03:43:24Z

Test build #116876 has finished for PR 26921 at commit 72b3307.

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

cloud-fan · 2020-01-17T05:43:40Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveCommandSuite.scala

+      val message = intercept[AnalysisException] {
+        sql("SHOW TBLPROPERTIES parquet_temp")
+      }.getMessage
+      assert(message.contains("parquet_temp is a temp view not table"))


This change LGTM, but can we add a note in the migration guide?

Sure, I will add it. Thanks!

cloud-fan · 2020-01-17T05:43:52Z

LGTM if tests pass

SparkQA · 2020-01-17T07:07:40Z

Test build #116889 has finished for PR 26921 at commit fb47ea9.

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

SparkQA · 2020-01-17T07:21:16Z

Test build #116890 has finished for PR 26921 at commit 1ff1dcd.

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

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

Test build #116905 has finished for PR 26921 at commit 23b743c.

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

cloud-fan · 2020-01-17T08:46:42Z

the last commit is just adding migration guide, and the previous commit passes all tests. I'm merging it to master, thanks!

dongjoon-hyun · 2020-01-17T17:51:34Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveCommandSuite.scala

@@ -130,10 +130,10 @@ class HiveCommandSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
  }

  test("show tblproperties for datasource table - errors") {
-    val message1 = intercept[NoSuchTableException] {
+    val message = intercept[AnalysisException] {


Ur, this sounds like another behavior change from Apache Spark 2.4.4.
cc @gatorsmile

Thanks for catching it! I will update the migration guide.

Created a follow-up PR: #27276

gatorsmile

CREATE VIEW gatorsmileView1
TBLPROPERTIES(prop1="3")
AS SELECT 1 AS c1, 2 AS c2

SHOW TBLPROPERTIES gatorsmileView1

This does not work after the migration. @imback82 @cloud-fan We need to double check whether the other migration causes the same regression.

gatorsmile · 2020-04-25T02:00:53Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

-    val catalog = sparkSession.sessionState.catalog
-
-    if (catalog.isTemporaryTable(table)) {
-      Seq.empty[Row]


We allow users to specify the table properties when creating temporary views.

CREATE TEMPORARY VIEW v1 TBLPROPERTIES(prop1 ="3") AS SELECT 1 AS c1, 2 AS c2

Instead of disallowing it, I think we should support it.

Sure I will create a PR for this.

Upon looking at this further, properties specified in CREATE TEMP VIEW is not used in CreateViewCommand, so we can revert the behavior back to the original one where we showed empty properties. What do you think?

I will still create a PR to support a permanent view, which is a regression.

sounds good to me. please remove the item in the migration guide too.

Thanks!

imback82 · 2020-04-25T03:11:08Z

This does not work after the migration. @imback82 @cloud-fan We need to double check whether the other migration causes the same regression.

Let me take a look.

imback82 · 2020-04-25T20:47:06Z

UnresolvedTable is used for SHOW TBLPROPERTIES and COMMENT ON TABLE, so only those should be affected. @cloud-fan can correct me if I am wrong here.

Since COMMENT ON TABLE is introduced in Spark 3.0, there should be no regression.

cloud-fan · 2020-04-27T06:16:57Z

It's unfortunate that many TABLE commands in Spark support view as well. We need to be careful when we migrate commands to v2 later.

@imback82 can you send a PR to make SHOW TBLPROPERTIES support views?

imback82 · 2020-04-27T20:14:10Z

I created a PR: #28375

### What changes were proposed in this pull request? This PR addresses two things: - `SHOW TBLPROPERTIES` should supports view (a regression introduced by #26921) - `SHOW TBLPROPERTIES` on a temporary view should return empty result (2.4 behavior instead of throwing `AnalysisException`. ### Why are the changes needed? It's a bug. ### Does this PR introduce any user-facing change? Yes, now `SHOW TBLPROPERTIES` works on views: ``` scala> sql("CREATE VIEW view TBLPROPERTIES('p1'='v1', 'p2'='v2') AS SELECT 1 AS c1") scala> sql("SHOW TBLPROPERTIES view").show(truncate=false) +---------------------------------+-------------+ |key |value | +---------------------------------+-------------+ |view.catalogAndNamespace.numParts|2 | |view.query.out.col.0 |c1 | |view.query.out.numCols |1 | |p2 |v2 | |view.catalogAndNamespace.part.0 |spark_catalog| |p1 |v1 | |view.catalogAndNamespace.part.1 |default | +---------------------------------+-------------+ ``` And for a temporary view: ``` scala> sql("CREATE TEMPORARY VIEW tview TBLPROPERTIES('p1'='v1', 'p2'='v2') AS SELECT 1 AS c1") scala> sql("SHOW TBLPROPERTIES tview").show(truncate=false) +---+-----+ |key|value| +---+-----+ +---+-----+ ``` ### How was this patch tested? Added tests. Closes #28375 from imback82/show_tblproperties_followup. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? This PR addresses two things: - `SHOW TBLPROPERTIES` should supports view (a regression introduced by #26921) - `SHOW TBLPROPERTIES` on a temporary view should return empty result (2.4 behavior instead of throwing `AnalysisException`. ### Why are the changes needed? It's a bug. ### Does this PR introduce any user-facing change? Yes, now `SHOW TBLPROPERTIES` works on views: ``` scala> sql("CREATE VIEW view TBLPROPERTIES('p1'='v1', 'p2'='v2') AS SELECT 1 AS c1") scala> sql("SHOW TBLPROPERTIES view").show(truncate=false) +---------------------------------+-------------+ |key |value | +---------------------------------+-------------+ |view.catalogAndNamespace.numParts|2 | |view.query.out.col.0 |c1 | |view.query.out.numCols |1 | |p2 |v2 | |view.catalogAndNamespace.part.0 |spark_catalog| |p1 |v1 | |view.catalogAndNamespace.part.1 |default | +---------------------------------+-------------+ ``` And for a temporary view: ``` scala> sql("CREATE TEMPORARY VIEW tview TBLPROPERTIES('p1'='v1', 'p2'='v2') AS SELECT 1 AS c1") scala> sql("SHOW TBLPROPERTIES tview").show(truncate=false) +---+-----+ |key|value| +---+-----+ +---+-----+ ``` ### How was this patch tested? Added tests. Closes #28375 from imback82/show_tblproperties_followup. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 3680303) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

imback82 added 2 commits December 16, 2019 22:53

initial checkin

5d8422d

minor cleanup

db311fd

imback82 changed the title ~~[SPARK-30282][SQL] UnresolvedV2Relation should be resolved to temp view first~~ [WIP][SPARK-30282][SQL] UnresolvedV2Relation should be resolved to temp view first Dec 17, 2019

imback82 changed the title ~~[WIP][SPARK-30282][SQL] UnresolvedV2Relation should be resolved to temp view first~~ [SPARK-30282][SQL] UnresolvedV2Relation should be resolved to temp view first Dec 17, 2019

imback82 added 2 commits December 17, 2019 00:24

update log message

fc2c5a3

update test

ae524c3

fix tests

467145f

imback82 commented Dec 18, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

imback82 added 3 commits January 7, 2020 21:08

Merge branch 'master' into consistnet_v2command

7b5816a

move to new framework

6de67d2

Merge branch 'master' into consistnet_v2command

517793f

imback82 changed the title ~~[SPARK-30282][SQL] UnresolvedV2Relation should be resolved to temp view first~~ [SPARK-30282][SQL] Integrate V2 commands with UnresolvedV2Relation into new resolution framework Jan 9, 2020

revert previous changes

a845488

imback82 commented Jan 9, 2020

View reviewed changes

cloud-fan reviewed Jan 9, 2020

View reviewed changes

cloud-fan mentioned this pull request Jan 9, 2020

[SPARK-30214][SQL] A new framework to resolve v2 commands #26847

Closed

cloud-fan mentioned this pull request Jan 13, 2020

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

Closed

address comments

4fe3e69

imback82 changed the title ~~[SPARK-30282][SQL] Integrate V2 commands with UnresolvedV2Relation into new resolution framework~~ [SPARK-30282][SQL] Migrate SHOW TBLPROPERTIES to new framework Jan 16, 2020

imback82 added 2 commits January 16, 2020 18:53

fix tests

fb47ea9

fix comments

1ff1dcd

cloud-fan reviewed Jan 17, 2020

View reviewed changes

Update sql-migration-guide.md

23b743c

cloud-fan closed this in 64fe192 Jan 17, 2020

dongjoon-hyun added the SQL label Jan 17, 2020

dongjoon-hyun reviewed Jan 17, 2020

View reviewed changes

gatorsmile reviewed Apr 25, 2020

View reviewed changes

imback82 mentioned this pull request Apr 28, 2020

[SPARK-30282][SQL][FOLLOWUP] SHOW TBLPROPERTIES should support views #28375

Closed

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

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

Uh oh!

Conversation

imback82 commented Dec 17, 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?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Dec 17, 2019

Uh oh!

SparkQA commented Dec 17, 2019

Uh oh!

SparkQA commented Dec 17, 2019

Uh oh!

SparkQA commented Dec 17, 2019

Uh oh!

SparkQA commented Dec 17, 2019

Uh oh!

imback82 commented Dec 18, 2019

Uh oh!

Uh oh!

cloud-fan commented Dec 19, 2019

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jan 9, 2020

Uh oh!

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

imback82 commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jan 12, 2020

Uh oh!

imback82 commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

cloud-fan commented Jan 17, 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!

imback82 commented Dec 17, 2019 •

edited

Loading

imback82 commented Jan 10, 2020 •

edited

Loading

gatorsmile Apr 25, 2020 •

edited

Loading

imback82 commented Apr 25, 2020 •

edited

Loading