[SPARK-46490][SQL] Require error classes in SparkThrowable sub-classes

[MaxGekk]

What changes were proposed in this pull request?

In the PR, I propose to create SparkThrowable sub-classes only with an error class by making the constructor with message private.

Why are the changes needed?

To improve user experience with Spark SQL by unifying error exceptions: the final goal is all Spark exception should contain an error class.

Does this PR introduce any user-facing change?

No since user's code shouldn't throw SparkThrowable sub-classes but it can if it depends on error message formats.

How was this patch tested?

By existing test test suites like:

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

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

No.

[dongjoon-hyun]

Could you make the CI happy? The failure messages seem to be related.

[MaxGekk]

I am trying to fix some issues in the PR #44468. So far, will convert this PR to a draft.

[MaxGekk]

Before this PR, we converted Java exception to Spark exception at the client side by passing the message as is (error class and message parameters was null or empty) like: NumberFormatException("invalid format 9hh99") -> SparkNumberFormatException("invalid format 9hh99", errorClass = null).

After the changes, the conversion still exists but I put the original message to a parameter:
NumberFormatException("invalid format 9hh99") -> SparkNumberFormatException(errorClass = "_LEGACY_ERROR_TEMP_3104", messageParameters = Map("message" -> "invalid format 9hh99"))

[MaxGekk]

Eventually, we should convert all Java exceptions to Spark exceptions but since Spark SQL still raises such exceptions, different error class will allow to identify which Java exceptions haven't been ported yet.

[MaxGekk]

@cloud-fan @dongjoon-hyun @HyukjinKwon @heyihong Please, review this PR.

[dongjoon-hyun]

Could you make the CI successful, @MaxGekk ?

[MaxGekk]

@dongjoon-hyun I guess Run / Run Spark on Kubernetes Integration test is not related to the changes, but I re-ran it again. I am observing the same failure in other PRs.

[heyihong]

LGTM with one nit

[heyihong]

On a second look, we may need some logics to handle cases that error classes are actually set

[heyihong]

Actually, this may need some special handling since params.errorClass and params.messageParameters may be set

[heyihong]

Maybe

errorClass = params.errorClass.orElse("_LEGACY_ERROR_TEMP_3104"),
messageParameters = params.errorClass match {
  case Some(_) => params.messageParameters
  case None => Map("message" -> params.message)
}

[MaxGekk]

How can it be set for non-SparkThrowable like here NumberFormatException?

[heyihong]

params.errorClass is None for non SparkThrowable?

https://github.com/databricks/runtime/blob/master/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala#L171-L174

[MaxGekk]

got it. will do.

[heyihong]

ditto

[heyihong]

Would be nice to update the test to cover both errorClass = Some(...) and errorClass = None

[MaxGekk]

updated

[MaxGekk]

Merging to master. Thank you, @heyihong and @dongjoon-hyun for review.