spark icon indicating copy to clipboard operation
spark copied to clipboard

[SPARK-49941][CORE] Rename `errorClass` to `condition` in errors of the JSON format

Open MaxGekk opened this issue 1 year ago • 5 comments

What changes were proposed in this pull request?

In the PR, I propose to rename the errorClass to condition in errors in the JSON formats: MINIMAL and STANDARD.

For example:

{
  "condition" : "DIVIDE_BY_ZERO",
  "sqlState" : "22012",
  "messageParameters" : { "config" : "CONFIG"}
}

The name condition was taken because it is used the SQL standard: Screenshot 2024-10-09 at 19 38 51 and no need extra words as a suffix or prefix in the context of error message format.

Why are the changes needed?

To follow new naming convention introduced by https://github.com/apache/spark/pull/44902.

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

By running the affected tests:

$ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite"

Was this patch authored or co-authored using generative AI tooling?

No.

MaxGekk avatar Oct 12 '24 12:10 MaxGekk

@srielau @panbingkun @nchammas @cloud-fan Could you review the PR, please.

MaxGekk avatar Oct 12 '24 19:10 MaxGekk

LGTM. nit: Do we need to update the following? image

panbingkun avatar Oct 14 '24 06:10 panbingkun

@panbingkun Thank you for review.

  1. UIUtils: The name errorClass is regexp group name. Not related to the changes.
  2. SQLJsonProtocolSuite: it is just an example in a test. It could handle old input.

MaxGekk avatar Oct 14 '24 09:10 MaxGekk

  1. UIUtils

Yeah!

  • Yes, it is indeed the group name of a regular expression. Not related to the changes, and calling it as errorClass will not affect the final functionality. (PS: As a follow-up, I'm not sure if we need to rename it to condiiton to reduce misunderstandings? Because from UT, it seems that its purpose is to obtain the value of condiiton (In the past, it was called errorClass) https://github.com/apache/spark/blob/488f68090b228b30ba4a3b75596c9904eef1f584/core/src/test/scala/org/apache/spark/ui/UIUtilsSuite.scala#L209-L211 (Because after this PR, I believe that in the error log, there should only be condiiton and no errorClass.)
  • Okay.

panbingkun avatar Oct 15 '24 01:10 panbingkun

@nchammas Thank you for your comment. I have updated PR's description and added reasons for the name.

MaxGekk avatar Oct 17 '24 09:10 MaxGekk

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

github-actions[bot] avatar Jun 03 '25 00:06 github-actions[bot]