[SPARK-36108][SQL] Refactor first set of 20 query parsing errors to use error classes by beliefer · Pull Request #33535 · apache/spark

beliefer · 2021-07-27T08:44:53Z

What changes were proposed in this pull request?

This PR refactor some exceptions in QueryParsingErrors to use error classes.

There are currently ~100 exceptions in this file; so this PR only focuses on the first set of 20.

Why are the changes needed?

To improve auditing, reduce duplication, and improve quality of error messages thrown from Spark, we should group them in a single JSON file (as discussed in the mailing list and introduced in SPARK-34920).

Does this PR introduce any user-facing change?

'No'.
Just use new error classes.

How was this patch tested?

Jenkins test.

SparkQA · 2021-07-27T10:10:36Z

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

HyukjinKwon · 2021-07-27T10:14:20Z

cc @karenfeng FYI

SparkQA · 2021-07-27T10:44:02Z

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

SparkQA · 2021-07-27T11:10:32Z

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

SparkQA · 2021-07-27T11:28:54Z

Test build #141693 has finished for PR 33535 at commit 32b28e0.

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

SparkQA · 2021-07-27T12:05:25Z

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

SparkQA · 2021-07-27T12:24:10Z

Test build #141695 has finished for PR 33535 at commit 9aea3eb.

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

karenfeng

Thanks for working on this! I left some suggestions to simplify the error classes. We should also make sure the SQLSTATEs match the ISO standard; if you're not sure what SQLSTATE fits, you can also leave it empty for now.

karenfeng · 2021-07-27T16:58:56Z

core/src/main/resources/error/error-classes.json

+    "sqlState" : "10001"
+  },
+  "INSERT_OVERWRITE_DIRECTORY_UNSUPPORTED" : {
+    "message" : [ "INSERT OVERWRITE DIRECTORY is not supported" ],


It'd be cleaner if we parametrized this error message to create a single error class representing simple operations that are unsupported in all cases, such as OPERATION_UNSUPPORTED: %s is not supported. Then we can collapse this with some of the error classes below.

karenfeng · 2021-07-27T17:01:52Z

core/src/main/resources/error/error-classes.json

+    "message" : [ "There must be at least one WHEN clause in a MERGE statement" ],
+    "sqlState" : "10008"
+  },
+  "NON_LAST_MATCHED_CLAUSE_OMIT_CONDITION" : {


We can also parametrize these two error messages to simplify auditing.

karenfeng · 2021-07-27T17:04:03Z

core/src/main/resources/error/error-classes.json

+  },
+  "INVALID_INSERT_INTO_CONTEXT" : {
+    "message" : [ "Invalid InsertIntoContext" ],
+    "sqlState" : "10001"


The SQLSTATEs should reflect those set in the ANSI/ISO standard; see https://github.com/apache/spark/blob/master/core/src/main/resources/error/README.md.

I'm not sure.

karenfeng · 2021-07-27T17:05:13Z

core/src/main/resources/error/error-classes.json

+    "message" : [ "INSERT OVERWRITE DIRECTORY is not supported" ],
+    "sqlState" : "10002"
+  },
+  "COLUMNS_ALIASES_NOT_ALLOWED_IN_OPERATION" : {


Grammar nit: Columns aliases -> Column aliases.

karenfeng · 2021-07-27T17:16:39Z

core/src/main/resources/error/error-classes.json

+    "message" : [ "Empty source for merge: you should specify a source table/subquery in merge." ],
+    "sqlState" : "10004"
+  },
+  "UNRECOGNIZED_MATCHED_ACTION" : {


Can we simplify this by parametrizing? Then we only need one for MATCHED/NOT_MATCHED.

karenfeng · 2021-07-27T17:18:05Z

core/src/main/resources/error/error-classes.json

+    "message" : [ "LATERAL cannot be used together with PIVOT in FROM clause" ],
+    "sqlState" : "10016"
+  },
+  "LATERAL_JOIN_WITH_NATURAL_JOIN_UNSUPPORTED" : {


I think we can merge these as well.

karenfeng · 2021-07-27T17:22:48Z

core/src/main/resources/error/error-classes.json

+    "sqlState" : "10020"
  }
-}
+}


This newline may be causing the test failures; we guarantee that the spacing is correct with a round trip read/write in the unit tests.

karenfeng · 2021-07-27T23:55:45Z

core/src/main/resources/error/error-classes.json

    "message" : [ "Writing job aborted" ],
    "sqlState" : "40000"
+  },
+  "INVALID_INSERT_INTO_CONTEXT" : {


These need to be in alphabetical order to pass the style tests.

beliefer · 2021-07-28T10:29:49Z

Thanks for working on this! I left some suggestions to simplify the error classes. We should also make sure the SQLSTATEs match the ISO standard; if you're not sure what SQLSTATE fits, you can also leave it empty for now.

Can you tell me how to know the SQLSTATEs?

SparkQA · 2021-07-28T11:35:00Z

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

SparkQA · 2021-07-28T12:29:11Z

Kubernetes integration test unable to build dist.

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

SparkQA · 2021-07-28T12:30:07Z

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

SparkQA · 2021-07-28T13:19:13Z

Test build #141771 has finished for PR 33535 at commit 91d05b9.

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

karenfeng · 2021-07-28T17:57:42Z

Hi @beliefer! The SQLSTATEs are in the ISO standard, but it's behind a paywall. You can look at free resources like the Wikipedia article, or the Oracle or DB2 manuals (although they often define their own classes and subclasses). I'll push a PR soon with a summarized version of the ISO standards.

SparkQA · 2021-07-28T19:46:35Z

Test build #141772 has finished for PR 33535 at commit 907e96d.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds no public classes.

karenfeng · 2021-07-28T20:39:41Z

core/src/main/resources/error/error-classes.json

  },
+  "COLUMN_ALIASES_NOT_ALLOWED_IN_OPERATION" : {
+    "message" : [ "Columns aliases are not allowed in %s." ],
+    "sqlState" : "10000"


See #33560 for the standard SQLSTATEs. I think this would be 42000; we'll do another pass before the 3.3 release to clean up any incorrect ones.

SparkQA · 2021-07-29T04:32:14Z

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

SparkQA · 2021-07-29T05:31:23Z

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

SparkQA · 2021-07-29T05:45:23Z

Test build #141803 has finished for PR 33535 at commit b1f97b0.

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

SparkQA · 2021-07-29T07:17:45Z

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

SparkQA · 2021-07-29T08:08:51Z

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

SparkQA · 2021-07-29T09:30:49Z

Test build #141812 has finished for PR 33535 at commit f3380a5.

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

beliefer · 2021-07-29T09:47:07Z

ping @karenfeng @HyukjinKwon Any other suggestion?

karenfeng

Thanks for doing this! LGTM. @HyukjinKwon, can you also take a look?

beliefer · 2021-08-02T04:28:08Z

ping @cloud-fan too.

cloud-fan · 2021-08-03T15:34:18Z

core/src/main/resources/error/error-classes.json

    "sqlState" : "42000"
  },
+  "COLUMN_ALIASES_NOT_ALLOWED_IN_OPERATION" : {
+    "message" : [ "Columns aliases are not allowed in %s." ],


columns aliases -> column aliases?

cloud-fan · 2021-08-03T15:58:29Z

core/src/main/resources/error/error-classes.json

    "sqlState" : "42000"
  },
+  "INVALID_INSERT_INTO_CONTEXT" : {
+    "message" : [ "Invalid InsertIntoContext" ],


This error message looks a big vague. What exactly the error is?

The message "Invalid InsertIntoContext" is thrown when withInsertInto does not match.

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

Line 267 in de62b5a

private def withInsertInto(

This is a user-facing error message, do we expect end-users to understand this message?

SparkQA · 2021-08-04T04:37:18Z

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

SparkQA · 2021-08-04T05:14:10Z

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

SparkQA · 2021-08-04T06:26:53Z

Test build #142018 has finished for PR 33535 at commit 3971047.

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

SparkQA · 2021-08-05T10:02:58Z

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

SparkQA · 2021-08-05T10:42:24Z

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

SparkQA · 2021-08-05T11:45:37Z

Test build #142078 has finished for PR 33535 at commit 86ea00d.

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

karenfeng · 2021-09-16T20:28:11Z

core/src/main/resources/error/error-classes.json

+  },
+  "TRANSFORM_WITH_SERDE_UNSUPPORTED" : {
+    "message" : [ "TRANSFORM with serde is only supported in hive mode" ],
+    "sqlState" : "42000"


This should probably be 0A000

karenfeng · 2021-09-16T20:28:35Z

core/src/main/resources/error/error-classes.json

+  "UNSUPPORTED_LATERAL_JOIN_TYPE" : {
+    "message" : [ "Unsupported LATERAL join type %s" ],
+    "sqlState" : "42000"
+  },


This should probably be 0A000

karenfeng · 2021-09-16T20:31:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

      case hiveDir: InsertOverwriteHiveDirContext =>
        val (isLocal, storage, provider) = visitInsertOverwriteHiveDir(hiveDir)
        InsertIntoDir(isLocal, storage, provider, query, overwrite = true)
-      case _ =>


Why was this removed?

karenfeng · 2021-09-16T20:33:14Z

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

-    new ParseException(s"Unrecognized matched action: ${ctx.matchedAction().getText}",
-      ctx.matchedAction())
+    new ParseException("UNRECOGNIZED_ACTION",
+      Array("matched", ctx.matchedAction().getText), ctx.matchedAction())


Can we make matched all caps? This may make it clearer.

karenfeng · 2021-09-16T20:33:20Z

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

-    new ParseException(s"Unrecognized not matched action: ${ctx.notMatchedAction().getText}",
-      ctx.notMatchedAction())
+    new ParseException("UNRECOGNIZED_ACTION",
+      Array("not matched", ctx.notMatchedAction().getText), ctx.notMatchedAction())


Can we make not matched all caps? This may make it clearer.

karenfeng · 2021-09-16T20:35:07Z

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

  def combinationQueryResultClausesUnsupportedError(ctx: QueryOrganizationContext): Throwable = {
-    new ParseException(
-      "Combination of ORDER BY/SORT BY/DISTRIBUTE BY/CLUSTER BY is not supported", ctx)
+    new ParseException("OPERATION_UNSUPPORTED",


We should probably have a separate error class for unsupported combinations.

karenfeng · 2021-09-16T20:46:32Z

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


  def lateralJoinWithNaturalJoinUnsupportedError(ctx: ParserRuleContext): Throwable = {
-    new ParseException("LATERAL join with NATURAL join is not supported", ctx)
+    new ParseException("OPERATION_UNSUPPORTED", Array("LATERAL join with NATURAL join"), ctx)


I think we can also make a separate error class here as well, maybe about how different join types are incompatible.

karenfeng · 2021-09-16T20:47:06Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

  private def intercept(sqlCommand: String, messages: String*): Unit =
    interceptParseException(parsePlan)(sqlCommand, messages: _*)

+  private def interceptWithErrorClass(sqlCommand: String, messages: String*)(


karenfeng · 2021-09-16T20:48:39Z

core/src/main/resources/error/error-classes.json

    "sqlState" : "42000"
  },
+  "INSERTED_VALUE_NUMBER_NOT_MATCH_FIELD_NUMBER" : {
+    "message" : [ "The number of inserted values cannot match the fields." ],


Nit: cannot -> does not

karenfeng · 2021-09-16T20:49:21Z

core/src/main/resources/error/error-classes.json

    "message" : [ "Invalid pivot column '%s'. Pivot columns must be comparable." ],
    "sqlState" : "42000"
  },
+  "INSERTED_VALUE_NUMBER_NOT_MATCH_FIELD_NUMBER" : {


The grammar is a bit strange. Maybe INSERTED_VALUE_AND_FIELD_NUMBER_MISMATCH?

github-actions · 2021-12-26T00:13:08Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

beliefer · 2021-12-26T07:40:34Z

@cloud-fan Could you help me to remove Stale label?

Refactor first set of 20 query parsing errors to use error classes

32b28e0

github-actions bot added CORE SQL labels Jul 27, 2021

beliefer added 2 commits July 27, 2021 18:00

Update code

bb4e1fb

Update code

9aea3eb

karenfeng reviewed Jul 27, 2021

View reviewed changes

beliefer added 2 commits July 28, 2021 16:27

Merge branch 'master' into SPARK-36108

c46d4a0

Update code

91d05b9

Update code

907e96d

karenfeng reviewed Jul 28, 2021

View reviewed changes

Update code

b1f97b0

Update code

f3380a5

karenfeng approved these changes Jul 30, 2021

View reviewed changes

cloud-fan reviewed Aug 3, 2021

View reviewed changes

Update code

3971047

Update code

86ea00d

karenfeng reviewed Sep 16, 2021

View reviewed changes

github-actions bot added the Stale label Dec 26, 2021

github-actions bot closed this Dec 27, 2021

Conversation

beliefer commented Jul 27, 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?

Uh oh!

SparkQA commented Jul 27, 2021

Uh oh!

HyukjinKwon commented Jul 27, 2021

Uh oh!

SparkQA commented Jul 27, 2021

Uh oh!

SparkQA commented Jul 27, 2021

Uh oh!

SparkQA commented Jul 27, 2021

Uh oh!

SparkQA commented Jul 27, 2021

Uh oh!

SparkQA commented Jul 27, 2021

Uh oh!

karenfeng left a comment

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!

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!

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!

beliefer commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

karenfeng commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 29, 2021

Uh oh!

SparkQA commented Jul 29, 2021

Uh oh!

SparkQA commented Jul 29, 2021

Uh oh!

SparkQA commented Jul 29, 2021

Uh oh!

SparkQA commented Jul 29, 2021

Uh oh!

SparkQA commented Jul 29, 2021

Uh oh!

beliefer commented Jul 29, 2021

Uh oh!

karenfeng left a comment

Choose a reason for hiding this comment

beliefer commented Aug 2, 2021 •

edited

Loading