-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-40663][SQL] Migrate execution errors onto error classes: _LEGACY_ERROR_TEMP_2000-2025 #38104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…CY_ERROR_TEMP_2000-2025
|
cc @MaxGekk could you take a look when you find some time. |
| }, | ||
| "_LEGACY_ERROR_TEMP_2001" : { | ||
| "message" : [ | ||
| "<message> If necessary set <ansiConfig> to false to bypass this error" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed a point after <message>?
Just in case, _LEGACY_ERROR_TEMP_2001 and _LEGACY_ERROR_TEMP_2000 are the same. Can you merge them, and reuse existing one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to merge this PR since the tests passed. Please, address this minor comment in your next PR #38108.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
Let me address them in my follow-ups. Thanks! :-)
|
+1, LGTM. Merging to master. |
| def ansiIllegalArgumentError(e: Exception): SparkIllegalArgumentException = { | ||
| new SparkIllegalArgumentException( | ||
| errorClass = "_LEGACY_ERROR_TEMP_2002", | ||
| messageParameters = Map("message" -> e.getMessage)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found a regression here. The previous error message has If necessary set ${SQLConf.ANSI_ENABLED.key} to false ... but it's gone now.
@itholic can you create a followup PR to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: follow-up here #38450
| struct<> | ||
| -- !query output | ||
| java.lang.IllegalArgumentException | ||
| Illegal input for day of week: xx. If necessary set spark.sql.ansi.enabled to false to bypass this error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test shows the regression I mentioned above.
| def invalidUrlError(url: UTF8String, e: URISyntaxException): Throwable = { | ||
| new IllegalArgumentException(s"Find an invalid url string ${url.toString}. " + | ||
| s"If necessary set ${SQLConf.ANSI_ENABLED.key} to false to bypass this error.", e) | ||
| def invalidUrlError(url: UTF8String, e: URISyntaxException): SparkIllegalArgumentException = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itholic Could you open a follow up and add a constructor to SparkIllegalArgumentException which accepts cause too, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let me create a follow-up soon.
What changes were proposed in this pull request?
This PR proposes to migrate 26 execution errors onto temporary error classes with the prefix
_LEGACY_ERROR_TEMP_2000to_LEGACY_ERROR_TEMP_2024.The error classes are prefixed with
_LEGACY_ERROR_TEMP_indicates the dev-facing error messages, and won't be exposed to end users.Why are the changes needed?
To speed-up the error class migration.
The migration on temporary error classes allow us to analyze the errors, so we can detect the most popular error classes.
Does this PR introduce any user-facing change?
No
How was this patch tested?