[SPARK-33685][SQL] Migrate DROP VIEW command to use UnresolvedView to resolve the identifier #30636

imback82 · 2020-12-07T02:50:25Z

What changes were proposed in this pull request?

This PR introduces UnresolvedView in the resolution framework to resolve the identifier.

This PR then migrates DROP VIEW to use UnresolvedView to resolve the table/view identifier. This allows consistent resolution rules (temp view first, etc.) to be applied for both v1/v2 commands. More info about the consistent resolution rule proposal can be found in JIRA or proposal doc.

Why are the changes needed?

To use UnresolvedView for view resolution. Note that there is no resolution behavior change with this PR.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Updated existing tests.

SparkQA · 2020-12-07T03:31:20Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36917/

SparkQA · 2020-12-07T03:46:26Z

Test build #132317 has finished for PR 30636 at commit 4d75250.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedView(
case class NoopCommand(multipartIdentifier: Seq[String]) extends Command
case class DropView(

imback82 · 2020-12-07T03:56:00Z

retest this please

SparkQA · 2020-12-07T03:58:06Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36917/

SparkQA · 2020-12-07T05:19:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36924/

SparkQA · 2020-12-07T05:43:37Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36924/

SparkQA · 2020-12-07T06:05:59Z

Test build #132324 has finished for PR 30636 at commit 4d75250.

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

SparkQA · 2020-12-07T06:30:46Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36930/

SparkQA · 2020-12-07T07:04:45Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36930/

SparkQA · 2020-12-07T08:34:59Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36940/

SparkQA · 2020-12-07T09:00:35Z

Test build #132330 has finished for PR 30636 at commit bc3d861.

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

SparkQA · 2020-12-07T09:10:41Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36940/

SparkQA · 2020-12-07T09:50:22Z

Test build #132340 has finished for PR 30636 at commit c23f61f.

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

imback82 · 2020-12-07T15:29:32Z

cc @cloud-fan

imback82 · 2020-12-07T15:29:37Z

retest this please

imback82 · 2020-12-07T16:07:21Z

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

    }
    assert(
-      e.getMessage.contains("Cannot drop a table with DROP VIEW. Please use DROP TABLE instead"))
+      e.getMessage.contains("dbx.tab1 is a table. 'DROP VIEW' expects a view."))


@cloud-fan do you think the message is regressing in terms of usefulness? One thing we can do is to introduce hint: Option[String] to UnresolvedView such that we can make the message as dbx.tab1 is a table. 'DROP VIEW' expects a view. Please use DROP TABLE instead.. WDYT?

SGTM. Do we need to do the same for table commands?

Yes. I will go thru commands that are using UnresolvedTable and add hints if necessary as a follow up PR.

And I will revisit #30475 (comment) for UnresolvedTableOrView.

SparkQA · 2020-12-07T16:16:23Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36978/

SparkQA · 2020-12-07T16:43:16Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36978/

cloud-fan · 2020-12-07T16:46:48Z

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

          .getOrElse(u)

+      case u @ UnresolvedView(NonSessionCatalogAndIdentifier(catalog, ident), cmd) =>
+        u.failAnalysis(


this seems like something that we should put in CheckAnalysis

Moved to CheckAnalysis.

cloud-fan · 2020-12-07T16:47:53Z

...lyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCommandsWithIfExists.scala

-      NoopDropTable(u.multipartIdentifier)
+      NoopCommand(u.multipartIdentifier)
+    case DropView(u: UnresolvedView, ifExists) if ifExists =>
+      NoopCommand(u.multipartIdentifier)


shall we add cmdName: String to NoopCommand?

Thanks, added.

SparkQA · 2020-12-07T17:45:24Z

Test build #132378 has finished for PR 30636 at commit c23f61f.

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

SparkQA · 2020-12-08T03:29:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36997/

SparkQA · 2020-12-08T03:36:14Z

Test build #132397 has finished for PR 30636 at commit 0ce0942.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait CheckAnalysis extends PredicateHelper with LookupCatalog
case class NoopCommand(

SparkQA · 2020-12-08T04:06:38Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36997/

SparkQA · 2020-12-08T05:28:33Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37009/

SparkQA · 2020-12-08T05:53:35Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37009/

cloud-fan · 2020-12-08T06:34:38Z

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

+            "because view support in v2 catalog has not been implemented yet. " +
+            s"$cmd expects a view.")
+
+      case u: UnresolvedView =>


do we have a clear rule about when hint should be used in error message?

The rule I am thinking is if an expected "type" of relation is not found. For example, UnresolvedTable is resolved to ResolvedView or UnresolvedView is resolved to ResolvedTable.

then probably we should make the name clearer: relationTypeMismatchHint

That sounds good, thanks!

SparkQA · 2020-12-08T06:44:11Z

Test build #132409 has finished for PR 30636 at commit addfa13.

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

cloud-fan

LGTM except one comment

SparkQA · 2020-12-08T12:22:29Z

Test build #132415 has finished for PR 30636 at commit 8decf27.

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

cloud-fan · 2020-12-08T14:06:14Z

This is definitely not related to SparkR. I'm merging it to master, thanks!

…ble for a better error message ### What changes were proposed in this pull request? This PR proposes to add `relationTypeMismatchHint` to `UnresolvedTable` so that if a relation is resolved to a view when a table is expected, a hint message can be included as a part of the analysis exception message. Note that the same feature is already introduced to `UnresolvedView` in #30636. This mostly affects `ALTER TABLE` commands where the analysis exception message will now contain `Please use ALTER VIEW as instead`. ### Why are the changes needed? To give a better error message. (The hint used to exist but got removed for commands that migrated to the new resolution framework) ### Does this PR introduce _any_ user-facing change? Yes, now `ALTER TABLE` commands include a hint to use `ALTER VIEW` instead. ``` sql("ALTER TABLE v SET SERDE 'whatever'") ``` Before: ``` "v is a view. 'ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]' expects a table. ``` After this PR: ``` "v is a view. 'ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]' expects a table. Please use ALTER VIEW instead. ``` ### How was this patch tested? Updated existing test cases to include the hint. Closes #31424 from imback82/better_error. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…ble for a better error message ### What changes were proposed in this pull request? This PR proposes to add `relationTypeMismatchHint` to `UnresolvedTable` so that if a relation is resolved to a view when a table is expected, a hint message can be included as a part of the analysis exception message. Note that the same feature is already introduced to `UnresolvedView` in apache#30636. This mostly affects `ALTER TABLE` commands where the analysis exception message will now contain `Please use ALTER VIEW as instead`. ### Why are the changes needed? To give a better error message. (The hint used to exist but got removed for commands that migrated to the new resolution framework) ### Does this PR introduce _any_ user-facing change? Yes, now `ALTER TABLE` commands include a hint to use `ALTER VIEW` instead. ``` sql("ALTER TABLE v SET SERDE 'whatever'") ``` Before: ``` "v is a view. 'ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]' expects a table. ``` After this PR: ``` "v is a view. 'ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]' expects a table. Please use ALTER VIEW instead. ``` ### How was this patch tested? Updated existing test cases to include the hint. Closes apache#31424 from imback82/better_error. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

initial commit -a

4d75250

github-actions bot added the SQL label Dec 7, 2020

fix tests

bc3d861

fix hive test

c23f61f

imback82 commented Dec 7, 2020

View reviewed changes

cloud-fan reviewed Dec 7, 2020

View reviewed changes

Address PR comments

0ce0942

Merge remote-tracking branch 'upstream/master' into drop_view_v2

f8d1da5

fix test

addfa13

cloud-fan reviewed Dec 8, 2020

View reviewed changes

fix test

78da830

cloud-fan approved these changes Dec 8, 2020

View reviewed changes

Address PR comment

8decf27

cloud-fan closed this in c05ee06 Dec 8, 2020

imback82 mentioned this pull request Feb 1, 2021

[SPARK-34317][SQL] Introduce relationTypeMismatchHint to UnresolvedTable for a better error message #31424

Closed

[SPARK-33685][SQL] Migrate DROP VIEW command to use UnresolvedView to resolve the identifier #30636

[SPARK-33685][SQL] Migrate DROP VIEW command to use UnresolvedView to resolve the identifier #30636

Uh oh!

Conversation

imback82 commented Dec 7, 2020

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

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

imback82 commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 7, 2020

Uh oh!

imback82 commented Dec 7, 2020

Uh oh!

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

SparkQA commented Dec 7, 2020

Uh oh!

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

SparkQA commented Dec 7, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imback82 Dec 8, 2020 • 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!

imback82 Dec 8, 2020 •

edited

Loading