You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/09/16 09:56:42 UTC
[GitHub] [spark] MaxGekk opened a new pull request, #37916: [WIP][SQL] Migrate parsing errors onto error classes
MaxGekk opened a new pull request, #37916:
URL: https://github.com/apache/spark/pull/37916
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r976788389
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2418,7 +2421,10 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
Literal(BigDecimal(raw).underlying())
} catch {
case e: AnalysisException =>
- throw new ParseException(e.message, ctx)
+ throw new ParseException(
+ errorClass = "_LEGACY_ERROR_TEMP_0061",
+ messageParameters = Map("msg" -> e.getMessage),
+ ctx)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r976107257
##########
core/src/main/resources/error/README.md:
##########
@@ -66,6 +66,8 @@ To access error fields, catch exceptions that extend `org.apache.spark.SparkThro
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.
Review Comment:
I will mark it as code. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r973207994
##########
sql/core/src/test/resources/sql-tests/results/comments.sql.out:
##########
@@ -132,20 +132,9 @@ select 1 as a
struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException
-
-Unclosed bracketed comment(line 3, pos 0)
-
-== SQL ==
-/*abc*/
-select 1 as a
-/*
-^^^
-
-2 as b
-/*abc*/
-, 3 as c
-
-/**/
+{
+ "errorClass" : "_LEGACY_ERROR_TEMP_055"
Review Comment:
Just in case, users will see the same error message by default (not JSON).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r973207413
##########
sql/core/src/test/resources/sql-tests/results/comments.sql.out:
##########
@@ -132,20 +132,9 @@ select 1 as a
struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException
-
-Unclosed bracketed comment(line 3, pos 0)
-
-== SQL ==
-/*abc*/
-select 1 as a
-/*
-^^^
-
-2 as b
-/*abc*/
-, 3 as c
-
-/**/
+{
+ "errorClass" : "_LEGACY_ERROR_TEMP_055"
Review Comment:
Yes. We don't provide context in many places. This is one of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r974242158
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2360,7 +2360,10 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
Literal(converter(rawStrippedQualifier))
} catch {
case e: NumberFormatException =>
- throw new ParseException(e.getMessage, ctx)
+ throw new ParseException(
Review Comment:
> we don't have to instantiate the exception instance in a central file if it's only used once.
Don't think we should make this excuse for such case. Wenchen, the purpose of the migration on the temporary error classes is to do that with as less effort as possible. That's why I didn't move the exception to a central file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
URL: https://github.com/apache/spark/pull/37916
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #37916:
URL: https://github.com/apache/spark/pull/37916#issuecomment-1251016904
> Can we update core/src/main/resources/error/README.md to mention this special error class naming prefix?
Let me do that in the next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r976111633
##########
core/src/main/resources/error/README.md:
##########
@@ -66,6 +66,8 @@ To access error fields, catch exceptions that extend `org.apache.spark.SparkThro
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.
Review Comment:
Sounds good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r973207413
##########
sql/core/src/test/resources/sql-tests/results/comments.sql.out:
##########
@@ -132,20 +132,9 @@ select 1 as a
struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException
-
-Unclosed bracketed comment(line 3, pos 0)
-
-== SQL ==
-/*abc*/
-select 1 as a
-/*
-^^^
-
-2 as b
-/*abc*/
-, 3 as c
-
-/**/
+{
+ "errorClass" : "_LEGACY_ERROR_TEMP_055"
Review Comment:
Yes. We don't provide context in many places (I mean QueryContext not line, pos). This is one of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37916:
URL: https://github.com/apache/spark/pull/37916#issuecomment-1250975885
Can we update `core/src/main/resources/error/README.md` to mention this special error class naming prefix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #37916:
URL: https://github.com/apache/spark/pull/37916#issuecomment-1253880709
@gatorsmile @cloud-fan @HyukjinKwon @gengliangwang @dongjoon-hyun @viirya I would like to merge this if there are no objections from your side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #37916:
URL: https://github.com/apache/spark/pull/37916#issuecomment-1249473715
cc @srielau @anchovYu Could you take a look at the PR, please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37916:
URL: https://github.com/apache/spark/pull/37916#issuecomment-1251888183
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r976096811
##########
core/src/main/resources/error/README.md:
##########
@@ -66,6 +66,8 @@ To access error fields, catch exceptions that extend `org.apache.spark.SparkThro
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.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #37916:
URL: https://github.com/apache/spark/pull/37916#issuecomment-1254691929
Merging to master. Thank you, @cloud-fan @itholic @entong @srielau for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srielau commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r973178090
##########
sql/core/src/test/resources/sql-tests/results/comments.sql.out:
##########
@@ -132,20 +132,9 @@ select 1 as a
struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException
-
-Unclosed bracketed comment(line 3, pos 0)
-
-== SQL ==
-/*abc*/
-select 1 as a
-/*
-^^^
-
-2 as b
-/*abc*/
-, 3 as c
-
-/**/
+{
+ "errorClass" : "_LEGACY_ERROR_TEMP_055"
Review Comment:
Did we loose the context?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #37916:
URL: https://github.com/apache/spark/pull/37916#issuecomment-1249570897
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r974207860
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2360,7 +2360,10 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
Literal(converter(rawStrippedQualifier))
} catch {
case e: NumberFormatException =>
- throw new ParseException(e.getMessage, ctx)
+ throw new ParseException(
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r974242158
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2360,7 +2360,10 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
Literal(converter(rawStrippedQualifier))
} catch {
case e: NumberFormatException =>
- throw new ParseException(e.getMessage, ctx)
+ throw new ParseException(
Review Comment:
> we don't have to instantiate the exception instance in a central file if it's only used once.
Don't think we should make this excuse for such case. Wenchen, the purpose of the migration on the temporary error classes is to do that with as less effort as possible. That's why I didn't move the exception to a central file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r974924548
##########
core/src/main/resources/error/README.md:
##########
@@ -13,7 +13,7 @@ and message parameters rather than an arbitrary error message.
3. Add a new class to `error-class.json`; keep in mind the invariants below.
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.
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r976096811
##########
core/src/main/resources/error/README.md:
##########
@@ -66,6 +66,8 @@ To access error fields, catch exceptions that extend `org.apache.spark.SparkThro
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.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] entong commented on a diff in pull request #37916: [SPARK-40473][SQL] Migrate parsing errors onto error classes
Posted by GitBox <gi...@apache.org>.
entong commented on code in PR #37916:
URL: https://github.com/apache/spark/pull/37916#discussion_r976770267
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2418,7 +2421,10 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
Literal(BigDecimal(raw).underlying())
} catch {
case e: AnalysisException =>
- throw new ParseException(e.message, ctx)
+ throw new ParseException(
+ errorClass = "_LEGACY_ERROR_TEMP_0061",
+ messageParameters = Map("msg" -> e.getMessage),
+ ctx)
Review Comment:
What should we do for re-throws? @MaxGekk @cloud-fan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org