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

imback82 · 2020-04-27T20:13:50Z

What changes were proposed in this pull request?

This PR addresses two things:

SHOW TBLPROPERTIES should support views (a regression introduced by [SPARK-30282][SQL] Migrate SHOW TBLPROPERTIES to new framework #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.

imback82 · 2020-04-27T20:15:32Z

cc: @cloud-fan @gatorsmile

SparkQA · 2020-04-28T02:05:17Z

Test build #121925 has finished for PR 28375 at commit a9a7b39.

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

cloud-fan · 2020-04-28T05:47:29Z

docs/sql-migration-guide.md

@@ -59,7 +59,7 @@ license: |

  - In Spark 3.0, you can use `ADD FILE` to add file directories as well. Earlier you could add only single files using this command. To restore the behavior of earlier versions, set `spark.sql.legacy.addSingleFileInAddFile` to `true`.

-  - In Spark 3.0, `SHOW TBLPROPERTIES` throws `AnalysisException` if the table does not exist. In Spark version 2.4 and below, this scenario caused `NoSuchTableException`. Also, `SHOW TBLPROPERTIES` on a temporary view causes `AnalysisException`. In Spark version 2.4 and below, it returned an empty result.
+  - In Spark 3.0, `SHOW TBLPROPERTIES` throws `AnalysisException` if the table does not exist. In Spark version 2.4 and below, this scenario caused `NoSuchTableException`.


maybe we can remove it. Exception class change doesn't worth a migration guide item.

This exception class change was brought up in this comment: #26921 (comment). Can I still remove it?

I don't think error message/exception type change is a breaking change. @dongjoon-hyun what do you think?

cloud-fan · 2020-04-28T05:49:43Z

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

@@ -193,18 +193,16 @@ class HiveCommandSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
    checkAnswer(sql("SHOW TBLPROPERTIES parquet_tab2('`prop2Key`')"), Row("prop2Val"))
  }

-  test("show tblproperties for spark temporary table - AnalysisException is thrown") {
+  test("show tblproperties for spark temporary table - empty row") {


can we move this test to show-tblproperties.sql as well?

OK, will do.

SparkQA · 2020-04-28T19:05:37Z

Test build #122008 has finished for PR 28375 at commit c4c02d7.

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

imback82 · 2020-04-28T19:48:37Z

retest this please

SparkQA · 2020-04-29T01:14:58Z

Test build #122017 has finished for PR 28375 at commit c4c02d7.

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

cloud-fan · 2020-04-29T07:05:55Z

sql/core/src/test/resources/sql-tests/inputs/show-tblproperties.sql

+-- create a temporary view with properties
+CREATE TEMPORARY VIEW tv TBLPROPERTIES('p1'='v1') AS SELECT 1 AS c1;
+
+-- Properties for a temporary view should be empty


BTW I'd treat this as a bug. I don't think anyone would expect SHOW TBLPROPERTIES to always return None for temp views.

This is an existing issue, we can deal with it later.

@cloud-fan I will create a PR for this. This can be misleading as you pointed out.

@cloud-fan There are few ways to address this:

Throw an exception if CREATE TEMPORARY VIEW contains properties since they are ignored anyway.

Throw an exception when SHOW TBLPROPERTIES are run on a temporary view in ShowTablePropertiesCommand.

Create UnresolvedTableOrPermanentView and use it for commands that don't support temporary views.

WDYT?

I'd vote for 3. We have many commands that named as XXX TABLE but can work for views.

cloud-fan · 2020-04-29T07:06:38Z

thanks, merging to master/3.0!

### 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>

initial commit

a9a7b39

probot-autolabeler bot added DOCS SQL labels Apr 27, 2020

imback82 mentioned this pull request Apr 27, 2020

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

Closed

cloud-fan reviewed Apr 28, 2020

View reviewed changes

Address PR comments

c4c02d7

cloud-fan reviewed Apr 29, 2020

View reviewed changes

cloud-fan closed this in 3680303 Apr 29, 2020

imback82 mentioned this pull request Jul 15, 2020

[SPARK-32327][SQL] SHOW TBLPROPERTIES is not allowed on temporary views #29127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

imback82 commented Apr 27, 2020 •

edited

Loading

Uh oh!

imback82 commented Apr 27, 2020

Uh oh!

SparkQA commented Apr 28, 2020

Uh oh!

cloud-fan Apr 28, 2020

Uh oh!

imback82 Apr 28, 2020

Uh oh!

cloud-fan Apr 28, 2020

Uh oh!

cloud-fan Apr 28, 2020

Uh oh!

imback82 Apr 28, 2020

Uh oh!

SparkQA commented Apr 28, 2020

Uh oh!

imback82 commented Apr 28, 2020

Uh oh!

SparkQA commented Apr 29, 2020

Uh oh!

cloud-fan Apr 29, 2020

Uh oh!

imback82 Jul 13, 2020

Uh oh!

imback82 Jul 14, 2020

Uh oh!

cloud-fan Jul 15, 2020

Uh oh!

cloud-fan commented Apr 29, 2020

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

imback82 commented Apr 27, 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!

imback82 commented Apr 27, 2020

Uh oh!

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

SparkQA commented Apr 28, 2020

Uh oh!

imback82 commented Apr 28, 2020

Uh oh!

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

cloud-fan commented Apr 29, 2020

Uh oh!

Uh oh!

imback82 commented Apr 27, 2020 •

edited

Loading