[SPARK-40473][SQL] Migrate parsing errors onto error classes by MaxGekk · Pull Request #37916 · apache/spark

MaxGekk · 2022-09-16T09:56:41Z

What changes were proposed in this pull request?

In the PR, I propose to migrate all parsing errors onto temporary error classes with the prefix _LEGACY_ERROR_TEMP_. The error message will not include the error classes, so, in this way we will preserve the existing behaviour.

Why are the changes needed?

The migration on temporary error classes allows to gather statistics about errors and detect most popular error classes.

Does this PR introduce any user-facing change?

No. The error messages should be almost the same by default.

How was this patch tested?

By running the modified test suites:

$ build/sbt "core/testOnly *SparkThrowableSuite"
$ build/sbt "test:testOnly *ExpressionParserSuite"

MaxGekk · 2022-09-16T14:57:57Z

cc @srielau @anchovYu Could you take a look at the PR, please.

srielau

This is very cool!
I have some general questions, observations:

You seem to assume < 1000 of these. But just this one PR consumes close to a hundred slots"
How do we keep the numbering straight for the next n PRs? I assume you used some tooling to whip this up so fast. So this was 0 => 1. How do we get from n => n + 1?

srielau · 2022-09-16T16:03:22Z

sql/core/src/test/resources/sql-tests/results/comments.sql.out

-
-/**/
+{
+  "errorClass" : "_LEGACY_ERROR_TEMP_055"


Did we loose the context?

Yes. We don't provide context in many places (I mean QueryContext not line, pos). This is one of them.

Just in case, users will see the same error message by default (not JSON).

MaxGekk · 2022-09-16T16:39:42Z

You seem to assume < 1000 of these. But just this one PR consumes close to a hundred slots"

Some time ago, I have counted the total number of exceptions to be ported onto error classes around 800. I don't see any problems to start using 4 digits after 999.

How do we keep the numbering straight for the next n PRs?

We have 3 types of errors, actually: parsing, compilation and execution. We could migrate them 1-by-1 using sequential numbers. Otherwise the PR will conflict a lot.

I assume you used some tooling to whip this up so fast.

I think it will be faster to manually port the exceptions without any tooling.

cloud-fan · 2022-09-19T12:47:44Z

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

    } catch {
      case e: NumberFormatException =>
-        throw new ParseException(e.getMessage, ctx)
+        throw new ParseException(


We should also mention it in readme.md or somewhere: we don't have to instantiate the exception instance in a central file if it's only used once. This can also simplify the error migration work.

cloud-fan · 2022-09-19T12:49:09Z

Can we update core/src/main/resources/error/README.md to mention this special error class naming prefix?

cloud-fan · 2022-09-20T06:09:10Z

core/src/main/resources/error/README.md

 4. Check if the exception type already extends `SparkThrowable`.
   If true, skip to step 6.
-5. Mix `SparkThrowable` into the exception.
+5. Mix `SparkThrowable` into the exception, and place it to Query.*Errors at least when it is invoked more than once.


I think we can change item 6 instead

Throw the exception with the error class and message parameters. If the same exception is thrown in several places, create a util function in a central place such as `QueryCompilationErrors.scala` to instantiate the exception.

cloud-fan · 2022-09-20T06:16:43Z

@dongjoon-hyun @viirya @HyukjinKwon The new error class name prefix _LEGACY_ERROR_TEMP_ proposed here kind of marks the error as developer-facing, not user-facing. Developers can still get the error class programmatically via the SparkThrowable interface, so that they can build error infra with it. End users won't see the error class in the message. This allows us to do the error migration very quickly, and we can refine the error classes and mark them as user-facing later (naming them properly, adding tests, etc.). What do you think?

itholic · 2022-09-21T06:44:53Z

core/src/main/resources/error/README.md


 Error classes are a succinct, human-readable representation of the error category.

+An uncategorized errors can be assigned to a legacy error class with the prefix _LEGACY_ERROR_TEMP_ and an unused sequential number, for instance _LEGACY_ERROR_TEMP_053.


nit: Maybe we should using \ in front of _LEGACY_ERROR_TEMP_ as an escape character ??

Otherwise, maybe it should look something like: LEGACY_ERROR_TEMP in the documents, which is missing leading and following underscore.

I will mark it as code. Thank you.

Sounds good!

MaxGekk · 2022-09-21T15:35:03Z

@gatorsmile @cloud-fan @HyukjinKwon @gengliangwang @dongjoon-hyun @viirya I would like to merge this if there are no objections from your side.

entong · 2022-09-21T16:47:10Z

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

+        throw new ParseException(
+          errorClass = "_LEGACY_ERROR_TEMP_0061",
+          messageParameters = Map("msg" -> e.getMessage),
+          ctx)


What should we do for re-throws? @MaxGekk @cloud-fan

Usually, we check either the exception belongs to an error class or not. If so, just re-throw it as is otherwise assign an error class. But in this PR, I just want to assign error classes and don't want to change existing behaviour.

MaxGekk · 2022-09-22T08:24:07Z

Merging to master. Thank you, @cloud-fan @itholic @entong @srielau for review.

### What changes were proposed in this pull request? In the PR, I propose to use `checkError()` to check the error class, message parameters and the query context. After the PR #37916, all parsing exceptions should have an error class. ### Why are the changes needed? 1. Checking of the error classes plus the query context improves the test coverage. 2. Eliminating the dependency of error messages. So, tech editors will be able to modify `error-classes.json` w/o fixing the test suite. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By running the modified test suite: ``` $ build/sbt "test:testOnly *ErrorParserSuite" ``` Closes #38204 from MaxGekk/intercept-parsing-error-class. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

MaxGekk added 2 commits September 16, 2022 12:39

Migrate parsing errors onto temporary error classes

04b0c47

Re-gen sql.out

72dbf22

github-actions bot added CORE SQL labels Sep 16, 2022

MaxGekk requested a review from gatorsmile September 16, 2022 10:13

MaxGekk added 2 commits September 16, 2022 16:59

Fix ExpressionParserSuite

a67095b

Exculde temp error classes

54f8ae1

MaxGekk changed the title ~~[WIP][SQL] Migrate parsing errors onto error classes~~ [WIP][SPARK-40473][SQL] Migrate parsing errors onto error classes Sep 16, 2022

MaxGekk requested review from HyukjinKwon, cloud-fan and gengliangwang September 16, 2022 14:57

MaxGekk changed the title ~~[WIP][SPARK-40473][SQL] Migrate parsing errors onto error classes~~ [SPARK-40473][SQL] Migrate parsing errors onto error classes Sep 16, 2022

MaxGekk marked this pull request as ready for review September 16, 2022 14:57

srielau approved these changes Sep 16, 2022

View reviewed changes

MaxGekk added 2 commits September 16, 2022 19:49

Re-gen sql.out

969ee9b

4 digits

ef3d20b

cloud-fan reviewed Sep 19, 2022

View reviewed changes

Update README.md

5944286

github-actions bot added the DOCS label Sep 19, 2022

Merge remote-tracking branch 'origin/master' into legacy-error-temp

c35c1ea

cloud-fan reviewed Sep 20, 2022

View reviewed changes

MaxGekk added 2 commits September 20, 2022 09:12

Re-gen sql.out

75a6841

Change item 6 in README

f94c8c7

Merge remote-tracking branch 'origin/master' into legacy-error-temp

62ea7bf

itholic reviewed Sep 21, 2022

View reviewed changes

Mark _LEGACY_ERROR_TEMP_ as code

686e060

Merge remote-tracking branch 'origin/master' into legacy-error-temp

31194da

entong reviewed Sep 21, 2022

View reviewed changes

cloud-fan approved these changes Sep 22, 2022

View reviewed changes

MaxGekk closed this in c4a0360 Sep 22, 2022

MaxGekk mentioned this pull request Sep 23, 2022

[SPARK-40540][SQL] Migrate compilation errors onto error classes #37973

Closed

MaxGekk mentioned this pull request Oct 11, 2022

[SPARK-40762][SQL][TESTS] Check error classes in ErrorParserSuite #38204

Closed


		Error classes are a succinct, human-readable representation of the error category.

		An uncategorized errors can be assigned to a legacy error class with the prefix _LEGACY_ERROR_TEMP_ and an unused sequential number, for instance _LEGACY_ERROR_TEMP_053.

Conversation

MaxGekk commented Sep 16, 2022 • 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!

MaxGekk commented Sep 16, 2022

Uh oh!

srielau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk Sep 16, 2022 • 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!

MaxGekk commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Sep 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Sep 20, 2022

Uh oh!

itholic Sep 21, 2022 • 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!

MaxGekk commented Sep 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MaxGekk commented Sep 16, 2022 •

edited

Loading

MaxGekk Sep 16, 2022 •

edited

Loading

itholic Sep 21, 2022 •

edited

Loading