[SPARK-33600][SQL] Group exception messages in execution/datasources/v2 #31619

karenfeng · 2021-02-23T06:23:11Z

What changes were proposed in this pull request?

This PR groups exception messages in execution/datasources/v2.

Why are the changes needed?

It will largely help with standardization of error messages and its maintenance.

Does this PR introduce any user-facing change?

No. Error messages remain unchanged.

How was this patch tested?

No new tests - pass all original tests to make sure it doesn't break any existing behavior.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-02-23T06:44:35Z

Test build #135368 has finished for PR 31619 at commit edb529c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-02-23T07:27:03Z

Test build #135370 has finished for PR 31619 at commit d004f5c.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

beliefer

There exists most Exception occusr in execution stage, so we should put them into QueryExecutionErros.

beliefer · 2021-02-25T10:10:57Z

...src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala

@@ -125,7 +126,7 @@ class InMemoryTableSessionCatalog extends TestV2SessionCatalogBase[InMemoryTable

        newTable
      case _ =>
-        throw new NoSuchTableException(ident)


beliefer · 2021-02-25T10:12:38Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceRDD.scala

@@ -83,7 +84,7 @@ private class PartitionIterator[T](reader: PartitionReader[T]) extends Iterator[

  override def next(): T = {
    if (!hasNext) {
-      throw new java.util.NoSuchElementException("End of stream")
+      throw QueryExecutionErrors.endOfStreamError()


NoSuchElementException is generated during execute and should be placed in QueryExecutionErrors.

I believe this is already located in QueryExecutionErrors.

I believe this is already located in QueryExecutionErrors.

Oh. I see. But fix the other comments please.

beliefer · 2021-02-25T10:12:53Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DropNamespaceExec.scala

      }
    } else if (!ifExists) {
-      throw new NoSuchNamespaceException(ns)
+      throw QueryCompilationErrors.noSuchNamespaceError(ns)


NoSuchNamespaceException is generated during execute and should be placed in QueryExecutionErrors.

As I mentioned in the separate comment, I grouped these into QueryCompilationErrors as they are a type of AnalysisException. Can you clarify?

You could look the description of root ticket.

If I read the root ticket correctly, all AnalysisExceptions should end up in QueryCompilationErrors:

A general rule of thumb for grouping exceptions: AnalysisException => QueryCompilationErrors SparkException, RuntimeException(UnsupportedOperationException, IllegalStateException...) => QueryExecutionErrors ParseException => QueryParsingErrors

Do you believe this rule of thumb does not apply here?

Yeah. But NoSuchNamespaceException occurs in execution stage. Although it extends AnalysisException.
cc @cloud-fan How about your suggestion?

Typically for exceptions that occur in the execution stage, i.e SparkPlan.execute, we want to treat these exceptions as query execution errors. But for commands, since they are executed eagerly, users can immediately see the errors after submitting the commands. It makes more sense to group them as query compilation errors. What do you think @beliefer

I got it. Thanks!

beliefer · 2021-02-25T10:13:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DropTableExec.scala

@@ -37,7 +37,7 @@ case class DropTableExec(
      invalidateCache()
      if (purge) catalog.purgeTable(ident) else catalog.dropTable(ident)
    } else if (!ifExists) {
-      throw new NoSuchTableException(ident)
+      throw QueryCompilationErrors.noSuchTableError(ident)


beliefer · 2021-02-25T10:13:23Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ReplaceTableExec.scala

@@ -74,10 +75,10 @@ case class AtomicReplaceTableExec(
          identifier, tableSchema, partitioning.toArray, tableProperties.asJava)
      } catch {
        case e: NoSuchTableException =>
-          throw new CannotReplaceMissingTableException(identifier, Some(e))


beliefer · 2021-02-25T10:14:52Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

@@ -278,7 +277,7 @@ class V2SessionCatalog(catalog: SessionCatalog)
      false

    case _ =>
-      throw new NoSuchNamespaceException(namespace)
+      throw QueryCompilationErrors.noSuchNamespaceError(namespace)


beliefer · 2021-02-25T10:15:04Z

...e/src/main/scala/org/apache/spark/sql/execution/datasources/v2/WriteToDataSourceV2Exec.scala

@@ -74,7 +74,7 @@ case class CreateTableAsSelectExec(
        return Nil
      }

-      throw new TableAlreadyExistsException(ident)
+      throw QueryCompilationErrors.tableAlreadyExistsError(ident)


beliefer · 2021-02-25T10:15:21Z

...e/src/main/scala/org/apache/spark/sql/execution/datasources/v2/WriteToDataSourceV2Exec.scala

@@ -200,10 +200,10 @@ case class AtomicReplaceTableAsSelectExec(
          ident, schema, partitioning.toArray, properties.asJava)
      } catch {
        case e: NoSuchTableException =>
-          throw new CannotReplaceMissingTableException(ident, Some(e))


beliefer · 2021-02-25T10:15:27Z

...e/src/main/scala/org/apache/spark/sql/execution/datasources/v2/WriteToDataSourceV2Exec.scala

@@ -153,7 +153,7 @@ case class ReplaceTableAsSelectExec(
      invalidateCache(catalog, table, ident)
      catalog.dropTable(ident)
    } else if (!orCreate) {
-      throw new CannotReplaceMissingTableException(ident)


beliefer · 2021-02-25T10:15:33Z

...e/src/main/scala/org/apache/spark/sql/execution/datasources/v2/WriteToDataSourceV2Exec.scala

@@ -109,7 +109,7 @@ case class AtomicCreateTableAsSelectExec(
        return Nil
      }

-      throw new TableAlreadyExistsException(ident)
+      throw QueryCompilationErrors.tableAlreadyExistsError(ident)


…3600 Signed-off-by: Karen Feng <karen.feng@databricks.com>

karenfeng · 2021-03-01T21:21:54Z

There exists most Exception occusr in execution stage, so we should put them into QueryExecutionErros.

Thanks for taking a look @beliefer! My impression for this grouping work is that the QueryCompilationErrors file should contain all AnalysisExceptions, regardless of where the exception was thrown. The errors that I grouped into QueryCompilationErrors wrapped AnalysisException, eg. NoSuchNamespaceException. Could you clarify why you think these exceptions should go into QueryExecutionErrors?

SparkQA · 2021-03-01T21:54:18Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40190/

SparkQA · 2021-03-01T22:11:47Z

Test build #135611 has finished for PR 31619 at commit 21bfce8.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-03-01T23:00:34Z

Test build #135613 has finished for PR 31619 at commit 3c0463a.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-03-01T23:25:23Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40192/

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-03-01T23:53:00Z

Test build #135614 has finished for PR 31619 at commit 862ce67.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-03-02T00:37:27Z

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

SparkQA · 2021-03-02T00:45:46Z

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

SparkQA · 2021-03-02T00:53:30Z

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

SparkQA · 2021-03-02T01:23:02Z

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

SparkQA · 2021-03-02T04:09:22Z

Test build #135615 has finished for PR 31619 at commit 847ee74.

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

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-03-04T19:27:15Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40350/

SparkQA · 2021-03-04T19:28:41Z

Test build #135767 has finished for PR 31619 at commit 36c0ada.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-03-05T03:05:56Z

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

SparkQA · 2021-03-05T03:32:36Z

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

SparkQA · 2021-03-05T03:56:04Z

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

SparkQA · 2021-03-05T04:05:15Z

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

allisonwang-db

Looks good. Left a few comments. We can remove the [WIP] in the PR title and change it to [SPARK-33600][SQL]

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

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

allisonwang-db · 2021-03-05T18:52:49Z

...c/main/scala/org/apache/spark/sql/catalyst/analysis/CannotReplaceMissingTableException.scala


 class CannotReplaceMissingTableException(
-    tableIdentifier: Identifier,
+    message: String,


We might not want to change the parameters for the class here.

def cannotReplaceMissingTableError( tableIdentifier: Identifier, cause: Option[Throwable] = None): Throwable = { new CannotReplaceMissingTableException(tableIdentifier, cause) }

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

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

allisonwang-db · 2021-03-05T19:19:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/execution/QueryExecutionException.scala

 class QueryExecutionException(message: String, cause: Throwable = null)
-  extends Exception(message, cause)
+  extends SparkException(message, cause)


It's good to know that there is a QueryExecutionException... We should leave the exception unchanged in PR to avoid unwanted side effects.

I moved this to inside catalyst so that it would be visible; I don't think this should be problematic as the package is still the same.

…3600 Signed-off-by: Karen Feng <karen.feng@databricks.com>

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-03-11T01:31:12Z

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

SparkQA · 2021-03-11T01:59:27Z

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

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-03-11T19:19:20Z

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

SparkQA · 2021-03-11T19:29:02Z

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

allisonwang-db

LGTM

…3600 Signed-off-by: Karen Feng <karen.feng@databricks.com>

karenfeng · 2021-03-17T21:24:21Z

@cloud-fan, can you take another look?

shaneknapp · 2021-03-17T22:23:18Z

test this please

SparkQA · 2021-03-17T23:57:06Z

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

SparkQA · 2021-03-18T00:28:46Z

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

karenfeng · 2021-03-23T17:28:57Z

@cloud-fan, can you take a look?

cloud-fan · 2021-03-25T16:15:28Z

thanks, merging to master!

Group exception messages in execution/datasources/v2

edb529c

Signed-off-by: Karen Feng <karen.feng@databricks.com>

github-actions bot added the SQL label Feb 23, 2021

Scalastyle

d004f5c

Signed-off-by: Karen Feng <karen.feng@databricks.com>

beliefer reviewed Feb 25, 2021

View reviewed changes

Merge branch 'master' of https://github.com/apache/spark into spark-3…

21bfce8

…3600 Signed-off-by: Karen Feng <karen.feng@databricks.com>

Fixup

3c0463a

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Fix truncate test

862ce67

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Fix scala style

847ee74

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Clarify grouping in file comment

36c0ada

Signed-off-by: Karen Feng <karen.feng@databricks.com>

karenfeng added 3 commits March 4, 2021 12:03

Clean up comments

7b36664

Signed-off-by: Karen Feng <karen.feng@databricks.com>

retrigger tests

b05c1d6

retrigger tests

1be2506

allisonwang-db reviewed Mar 5, 2021

View reviewed changes

karenfeng changed the title ~~[WIP][SPARK-33600] Group exception messages in execution/datasources/v2~~ [SPARK-33600][SQL] Group exception messages in execution/datasources/v2 Mar 10, 2021

karenfeng added 2 commits March 10, 2021 13:03

Merge branch 'master' of https://github.com/apache/spark into spark-3…

933dc50

…3600 Signed-off-by: Karen Feng <karen.feng@databricks.com>

Address comments

86a68a3

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Retrigger tests

6e9e73d

Signed-off-by: Karen Feng <karen.feng@databricks.com>

allisonwang-db approved these changes Mar 11, 2021

View reviewed changes

Merge branch 'master' of https://github.com/apache/spark into spark-3…

f02ed14

…3600 Signed-off-by: Karen Feng <karen.feng@databricks.com>

Trigger tests

d7a0f06

cloud-fan approved these changes Mar 25, 2021

View reviewed changes

cloud-fan closed this in 0d91f9c Mar 25, 2021

[SPARK-33600][SQL] Group exception messages in execution/datasources/v2 #31619

[SPARK-33600][SQL] Group exception messages in execution/datasources/v2 #31619

Conversation

karenfeng commented Feb 23, 2021

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?

SparkQA commented Feb 23, 2021

SparkQA commented Feb 23, 2021

beliefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliefer Feb 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenfeng commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 4, 2021

SparkQA commented Mar 4, 2021

SparkQA commented Mar 5, 2021

SparkQA commented Mar 5, 2021

SparkQA commented Mar 5, 2021

SparkQA commented Mar 5, 2021

allisonwang-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 11, 2021

SparkQA commented Mar 11, 2021

SparkQA commented Mar 11, 2021

SparkQA commented Mar 11, 2021

allisonwang-db left a comment

Choose a reason for hiding this comment

karenfeng commented Mar 17, 2021

shaneknapp commented Mar 17, 2021

SparkQA commented Mar 17, 2021

SparkQA commented Mar 18, 2021

karenfeng commented Mar 23, 2021

cloud-fan commented Mar 25, 2021

beliefer Feb 25, 2021 •

edited