-
Notifications
You must be signed in to change notification settings - Fork 29.1k
[SPARK-56029] Optional sub error conditions #54861
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
Open
srielau
wants to merge
7
commits into
apache:master
Choose a base branch
from
srielau:SPARK-56029-optional-error-conditions
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+230
−46
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4886596
[SPARK-56029] Optional sub error conditions
srielau 04c21b2
Update common/utils/src/main/scala/org/apache/spark/ErrorClassesJSONR…
srielau 479c973
Update sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionS…
srielau 9a408f5
Update common/utils/src/main/resources/error/README.md
srielau 97973fa
black
srielau e328f1f
Fix python and connect
srielau 111251c
Fix linter
srielau File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
while I understand the motivation, I think this might be too lenient if left just like this. it would be nice to have an "opt-in" mechanism, so we have some kind of an indication that this is a desired behavior for a specific error condition. otherwise, we are very silently converting all error conditions to a new behavior - over time this might lead to subclasses being effectively unused, since the path of least resistance is always the main condition.
I think it's not too complex to mitigate though. We can make an opt-in mechanism in
error-conditions.json, something like:And then use
subClassOptionalinErrorClassesJSONReaderto relax the condition when the value is set totrue.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'm open to to that. Let's see what other think. Note that this is NOT what we do for SQL Scripting handlers though.
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'm not sure how is this directly related to handlers? Here, we are changing how errors are raised, not handled. Regarding handler finding mechanism, you already explained that it would handle the new behavior, right?
If you are talking about not having SIGNAL in SQL Scripting, that's a gap that needs to be addressed. Once we add SIGNAL, I guess it should follow the same behavior as
raise_errordoes.But that's a separate matter - is there any special behavior that we should be aware of when supporting SIGNAL?
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.
"subClassOptional": truelooks wrong to me. We should always allow the error raising side to skip the sub error condition when not needed (we should still check error parameters to be an exact match to avoid mistakes).while for test, we should still do extract match by default (if error was thrown with sub error condition,
checkErrorshould useMAIN.SUB. if error was not thrown with sub error condition,checkErrorshould useMAIN). I'm OK to special case some widely tested errors likeTABLE_OR_VIEW_NOT_FOUND: when it starts to have error conditions, we can avoid touching many tests. SocheckErrorshould have a new "strict" flag, which by default is true (except for the special cases). Tests can override it if needed, and it's a test behavior, not a property of the error (allow optional sub error condition or not).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 can't do that. If I need to change code to supend the proper check, I may as well change the code to do the proper check.
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 do agree that I should always be able to raise an error without a subcondition. n excpliict default should not be necessary.
Uh oh!
There was an error while loading. Please reload this page.
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.
FWIW, thsi is what I'm trying to avoid:

So the idea is to have: TABLE_OR_VIEW_NOT_FOUND.WITH_PATH and use it where it's worth testing (or roll it out in chunks). But I cannot break existing uisers just testing for TABLE_OR_VIEW_NOT_FOUND. That's a breaking change.