[SPARK-37214][SQL] Fail query analysis earlier with invalid identifiers by cloud-fan · Pull Request #34490 · apache/spark

cloud-fan · 2021-11-05T08:57:42Z

What changes were proposed in this pull request?

This is a followup of #31427 , which introduced two issues:

When we lookup spark_catalog.t, we failed earlier with The namespace in session catalog must have exactly one name part before that PR, now we fail very late in CheckAnalysis with NoSuchTableException
The error message is a bit confusing now. We report Table t not found even if table t exists.

This PR fixes the 2 issues.

Why are the changes needed?

save analysis time and improve error message

Does this PR introduce any user-facing change?

no

How was this patch tested?

updated test

cloud-fan · 2021-11-05T09:00:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

We can't throw NoSuchTableException here. The CatalogV2Utils.loadTable swallows NoSuchTableException and delays the error to CheckAnalysis, and we lost the actual error message.

cloud-fan · 2021-11-05T09:01:46Z

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

This is already tested in https://github.com/apache/spark/pull/34490/files#diff-eeb429a8e3eb55228451c8dbc2fccca044836be608d62e9166561b005030c940R2317

cloud-fan · 2021-11-05T09:02:05Z

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

It's a parser error, I moved the test to https://github.com/apache/spark/pull/34490/files#diff-0db82228a944d6070203b6ac834c3c8d9b16b985fc3ff319ed7408fc3b6a6c23R363

cloud-fan · 2021-11-05T09:02:22Z

cc @holdenk @HyukjinKwon @viirya @imback82

SparkQA · 2021-11-05T09:59:12Z

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

SparkQA · 2021-11-05T11:00:01Z

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

SparkQA · 2021-11-05T11:56:25Z

Test build #144926 has finished for PR 34490 at commit f634d80.

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

SparkQA · 2021-11-05T13:35:59Z

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

SparkQA · 2021-11-05T14:36:23Z

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

SparkQA · 2021-11-05T17:54:15Z

Test build #144932 has finished for PR 34490 at commit deb664d.

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

imback82 · 2021-11-06T04:55:02Z

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

      }.getMessage
-      assert(message.contains(
-        "Table or view not found: a.b.c.tbl"))
+      assert(message.contains("requires a single-part namespace"))


Hmm, this could be a little misleading since this sounds like SHOW COLUMNS IN is only supported for the session catalog? (although it is currently not supported for v2 catalogs.) This may be OK until we start supporting this command for v2 catalogs.

dongjoon-hyun · 2021-11-07T23:16:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

-      s"V2 session catalog requires a single-part namespace: ${ident.quoted}")
+  def requiresSinglePartNamespaceError(ns: Seq[String]): Throwable = {
+    new AnalysisException(
+      "spark_catalog requires a single-part namespace, but got " + ns.mkString("[", ", ", "]"))


If you don't mind, can we use SESSION_CATALOG_NAME instead of spark_catalog?

- "spark_catalog requires a single-part namespace, but got " + ns.mkString("[", ", ", "]")) + s"${SESSION_CATALOG_NAME} requires a single-part namespace, but got " + + ns.mkString("[", ", ", "]"))

dongjoon-hyun

+1, LGTM (with one nit comment and also agree with @imback82 's comment.)

dongjoon-hyun · 2021-11-07T23:18:50Z

BTW, @cloud-fan . According to the JIRA type, is this targeting only Apache Spark 3.3.0?

cloud-fan · 2021-11-08T03:16:20Z

is this targeting only Apache Spark 3.3.0?

This just fixes error message, so we don't have to backport. But since the change is small, I think it should be safe to backport to 3.2 at least.

SparkQA · 2021-11-08T04:44:44Z

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

SparkQA · 2021-11-08T05:28:57Z

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

cloud-fan · 2021-11-08T05:33:28Z

thanks for review, merging to master/3.2!

This is a followup of #31427 , which introduced two issues: 1. When we lookup `spark_catalog.t`, we failed earlier with `The namespace in session catalog must have exactly one name part` before that PR, now we fail very late in `CheckAnalysis` with `NoSuchTableException` 2. The error message is a bit confusing now. We report `Table t not found` even if table `t` exists. This PR fixes the 2 issues. save analysis time and improve error message no updated test Closes #34490 from cloud-fan/table. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 8ab9d63) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

SparkQA · 2021-11-08T08:46:17Z

Test build #144978 has finished for PR 34490 at commit 3f38d60.

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

HyukjinKwon · 2021-11-18T02:01:22Z

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

    comparePlans(parsed2, expected2)
+
+    val v3 = "CREATE TEMPORARY VIEW a.b AS SELECT 1"
+    intercept(v3, "It is not allowed to add database prefix")


@cloud-fan, seems like this test is flaky:

https://github.com/apache/spark/runs/4209199644
https://github.com/apache/spark/runs/4235793982
https://github.com/apache/spark/runs/4223996038

See also SPARK-37308

I thought this was an old test so didn't take an action but just noticed that it's pretty new. Probably we should revert this one - seems pretty frequently failing.

Im gonna partially revert this change alone for now because other tests seem passing fine.

…alyzer error ### What changes were proposed in this pull request? followup of #34490 in branch-3.2, which moves the test for checking `notAllowedToAddDBPrefixForTempViewError` in the parser phase. But it only passes in master. In branch-3.2, the error happens in the analyzer phase. diverge happens in PR #34283 ### Why are the changes needed? fix test ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? pass the restored test. Closes #34633 from linhongliu-db/SPARK-37214-3.2. Authored-by: Linhong Liu <linhong.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…flakiness ### What changes were proposed in this pull request? This PR reverts one of the tests added at #34490 which is (pretty seriously) flaky. ### Why are the changes needed? Other tests seem passing fine so it's likely test specific issue. so only test is proposed to be reverted for now. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI in this PR should test this out. Closes #34639 from HyukjinKwon/SPARK-37308. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

This is a followup of apache#31427 , which introduced two issues: 1. When we lookup `spark_catalog.t`, we failed earlier with `The namespace in session catalog must have exactly one name part` before that PR, now we fail very late in `CheckAnalysis` with `NoSuchTableException` 2. The error message is a bit confusing now. We report `Table t not found` even if table `t` exists. This PR fixes the 2 issues. save analysis time and improve error message no updated test Closes apache#34490 from cloud-fan/table. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 8ab9d63) Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e55bab5) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…alyzer error ### What changes were proposed in this pull request? followup of apache#34490 in branch-3.2, which moves the test for checking `notAllowedToAddDBPrefixForTempViewError` in the parser phase. But it only passes in master. In branch-3.2, the error happens in the analyzer phase. diverge happens in PR apache#34283 ### Why are the changes needed? fix test ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? pass the restored test. Closes apache#34633 from linhongliu-db/SPARK-37214-3.2. Authored-by: Linhong Liu <linhong.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

This is a followup of apache#31427 , which introduced two issues: 1. When we lookup `spark_catalog.t`, we failed earlier with `The namespace in session catalog must have exactly one name part` before that PR, now we fail very late in `CheckAnalysis` with `NoSuchTableException` 2. The error message is a bit confusing now. We report `Table t not found` even if table `t` exists. This PR fixes the 2 issues. save analysis time and improve error message no updated test Closes apache#34490 from cloud-fan/table. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 8ab9d63) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…alyzer error ### What changes were proposed in this pull request? followup of apache#34490 in branch-3.2, which moves the test for checking `notAllowedToAddDBPrefixForTempViewError` in the parser phase. But it only passes in master. In branch-3.2, the error happens in the analyzer phase. diverge happens in PR apache#34283 ### Why are the changes needed? fix test ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? pass the restored test. Closes apache#34633 from linhongliu-db/SPARK-37214-3.2. Authored-by: Linhong Liu <linhong.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

This is a followup of apache#31427 , which introduced two issues: 1. When we lookup `spark_catalog.t`, we failed earlier with `The namespace in session catalog must have exactly one name part` before that PR, now we fail very late in `CheckAnalysis` with `NoSuchTableException` 2. The error message is a bit confusing now. We report `Table t not found` even if table `t` exists. This PR fixes the 2 issues. save analysis time and improve error message no updated test Closes apache#34490 from cloud-fan/table. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 8ab9d63) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…alyzer error ### What changes were proposed in this pull request? followup of apache#34490 in branch-3.2, which moves the test for checking `notAllowedToAddDBPrefixForTempViewError` in the parser phase. But it only passes in master. In branch-3.2, the error happens in the analyzer phase. diverge happens in PR apache#34283 ### Why are the changes needed? fix test ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? pass the restored test. Closes apache#34633 from linhongliu-db/SPARK-37214-3.2. Authored-by: Linhong Liu <linhong.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

github-actions bot added the SQL label Nov 5, 2021

cloud-fan commented Nov 5, 2021

View reviewed changes

Fail query analysis earlier with invalid identifiers

deb664d

cloud-fan force-pushed the table branch from f634d80 to deb664d Compare November 5, 2021 12:09

viirya approved these changes Nov 6, 2021

View reviewed changes

imback82 approved these changes Nov 6, 2021

View reviewed changes

dongjoon-hyun reviewed Nov 7, 2021

View reviewed changes

dongjoon-hyun approved these changes Nov 7, 2021

View reviewed changes

address comment

3f38d60

dongjoon-hyun approved these changes Nov 8, 2021

View reviewed changes

cloud-fan closed this in 8ab9d63 Nov 8, 2021

linhongliu-db mentioned this pull request Nov 17, 2021

[SPARK-37214][SQL][3.2][FOLLOWUP] No db prefix for temp view is an analyzer error #34633

Closed

HyukjinKwon reviewed Nov 18, 2021

View reviewed changes

HyukjinKwon mentioned this pull request Nov 18, 2021

[SPARK-37308][SQL][TESTS] Partially revert SPARK-37214's test due to flakiness #34639

Closed

Conversation

cloud-fan commented Nov 5, 2021 • 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 Nov 5, 2021

Uh oh!

SparkQA commented Nov 5, 2021

Uh oh!

SparkQA commented Nov 5, 2021

Uh oh!

SparkQA commented Nov 5, 2021

Uh oh!

SparkQA commented Nov 5, 2021

Uh oh!

SparkQA commented Nov 5, 2021

Uh oh!

SparkQA commented Nov 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Nov 7, 2021

Uh oh!

cloud-fan commented Nov 8, 2021

Uh oh!

SparkQA commented Nov 8, 2021

Uh oh!

SparkQA commented Nov 8, 2021

Uh oh!

cloud-fan commented Nov 8, 2021

Uh oh!

SparkQA commented Nov 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Nov 18, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloud-fan commented Nov 5, 2021 •

edited

Loading

HyukjinKwon Nov 18, 2021 •

edited

Loading