[SPARK-40530][SQL] Add error-related developer APIs#37969
[SPARK-40530][SQL] Add error-related developer APIs#37969cloud-fan wants to merge 2 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
not related to this PR, but I found this method is not called anywhere.
There was a problem hiding this comment.
this method is called no where.
There was a problem hiding this comment.
@MaxGekk I think this is a mistake. The STANDARD mode is a combination of PRETTY and MINIMAL mode: it can get all the structured information, as well as the original pretty message.
There was a problem hiding this comment.
The STANDARD mode is a combination of PRETTY and MINIMAL mode
From where it comes? In the current implementation, the info is orthogonal. If you need, you can substitute params yourself as you want.
There was a problem hiding this comment.
Makes sense, we should allow the caller side to do substitution as they want.
| * A reader to load error information from one or more JSON files. Note that, if one error appears | ||
| * in more than one JSON files, the latter wins. | ||
| */ | ||
| @DeveloperApi |
There was a problem hiding this comment.
As a developer API, do we need to briefly describe the error JSON format here?
There was a problem hiding this comment.
Added the reference to the readme file of the error framework.
| val subErrorClass = errorClasses.tail.headOption | ||
| val errorInfo = errorInfoMap.getOrElse( | ||
| mainErrorClass, | ||
| throw SparkException.internalError(s"Cannot find error class '$errorClass'")) |
There was a problem hiding this comment.
Here should be mainErrorClass? in the internal error message?
There was a problem hiding this comment.
I think it's better to include more information and put the full error class (main + sub)
| } else { | ||
| val errorSubInfo = errorInfo.subClass.get.getOrElse( | ||
| subErrorClass.get, | ||
| throw SparkException.internalError(s"Cannot find sub error class '${subErrorClass.get}'")) |
There was a problem hiding this comment.
Why not keep IllegalArgumentException?
There was a problem hiding this comment.
Because this means a bug and it's better to throw intenal error.
|
|
||
| test("Check if error class is missing") { | ||
| val ex1 = intercept[IllegalArgumentException] { | ||
| val ex1 = intercept[SparkException] { |
There was a problem hiding this comment.
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
Looks good overall. Just a few minor comments.
| | } | ||
| |} | ||
| |""".stripMargin) | ||
| val reader = new ErrorClassesJsonReader(Seq(errorJsonFilePath.toUri.toURL, json.toURL)) |
There was a problem hiding this comment.
Do you use the existing JSON file? How can you guarantee that one JSON overrides the standard one? Maybe you will add small one for testing to check that it overrides error-classes.json?
There was a problem hiding this comment.
This test uses 2 JSON files: the existing official one and a temporary one which was written in this test.
|
thanks for review, merging to master! |
3 participants
What changes were proposed in this pull request?
Third-party Spark plugins may define their own errors using the same framework as Spark: put error definition in json files. This PR moves some error-related code to a new file and marks them as developer APIs, so that others can reuse them instead of writing its own json reader.
Why are the changes needed?
make it easier for Spark plugins to define errors.
Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests