[SPARK-58999][SPARK] Allow a bare error class when it defines sub-classes - #58282
[SPARK-58999][SPARK] Allow a bare error class when it defines sub-classes#58282ala wants to merge 2 commits into
Conversation
ErrorClassesJsonReader.getMessageTemplate asserted that a sub-class is provided if and only if the main error class defines sub-classes, so it was impossible to render an error raised with just the main class (e.g. MAIN_WITH_SUBCLASS) when sub-classes exist. This also propagated to getErrorMessage, getMessageParameters, and SparkThrowableHelper. Drop the assertion: a bare main class now returns just its main message template even when sub-classes are defined. Requesting a sub-class of a main class that has none, or an unknown sub-class, now raises a clear internal error instead of a NoSuchElementException from Option.get. Add a test in SparkThrowableSuite covering getMessageTemplate and getMessageParameters.
Can you share some motivating examples? |
|
Sure! I work most often with Delta, which uses Spark's error message infra, so here's two recent examples from that project: DELTA_NON_PARTITION_COLUMN_ABSENT.ALL_PARTITION_COLUMNShttps://github.com/delta-io/delta/pull/7416/changes We had to settle on an awkwardly redundant message, because the we couldn't raise the main error class without a subclass:
"DELTA_NON_PARTITION_COLUMN_ABSENT" : {
"message" : [
"Data written into Delta needs to contain at least one non-partitioned column."
],
"subClass" : {
"ALL_PARTITION_COLUMNS" : {
"message" : [
"All of the provided columns are partition columns."
]
},
"NULL_TYPE_COLUMNS_DROPPED" : {
"message" : [
"Columns which are of NullType have been dropped."
]
}
},
"sqlState" : "KD005"
},DELTA_UNSUPPORTED_DROP_COLUMNRight now a complex message is hard-coded in the Scala code. We would like to move it into a subclass in JSON, but we don't want to create an awkward "no advice" default. Hence, we're blocked by this change. protected def columnMappingAdviceMessage(
requiredProtocol: Protocol = ColumnMappingTableFeature.minProtocolVersion): String = {
val readerVersion = requiredProtocol.minReaderVersion
val writerVersion = requiredProtocol.minWriterVersion
s"""
|Please enable Column Mapping on your Delta table with mapping mode 'name'.
|You can use one of the following commands.
|
|ALTER TABLE table_name SET TBLPROPERTIES ('delta.columnMapping.mode' = 'name')
|
|Note, if your table is not on the required protocol version it will be upgraded.
|Column mapping requires at least protocol ($readerVersion, $writerVersion)
|""".stripMargin
} def dropColumnNotSupported(suggestUpgrade: Boolean): Throwable = {
val adviceMsg = if (suggestUpgrade) columnMappingAdviceMessage() else ""
new DeltaAnalysisException("DELTA_UNSUPPORTED_DROP_COLUMN", Array(adviceMsg))
} "DELTA_UNSUPPORTED_DROP_COLUMN" : {
"message" : [
"DROP COLUMN is not supported for your Delta table. <advice>"
],
"sqlState" : "0AKDC"
}, |
Wrap the internal-error message for an unknown sub-class onto its own line so the source line no longer exceeds the 100-character scalastyle limit.
I don't see the problem with this setup. To me this reads: "A non-partitioned column could be absent for a number of reasons. a) The entire table is partitioned columns. b) There are non-partitioned columns, but they are of
If the advice is not always applicable, what's wrong with simply leaving it empty? We already have several error conditions with an optional hint. Some examples:
|
|
@HyukjinKwon - Thoughts on my comment just above? If you disagree with my assessment I would be interested in understanding why. |
|
My understanding is that the purpose of the JSON-based infra for the errors classes is to put the content of all the error messages in one central human-readable place. This enables, for example, quick audit of all the error messages and their contents. As features, configs, etc. are added or retired, we can more easily tweak the messaging to guide the users towards or away from them. It also enables a translation layer to be implemented, so that the users can receive localized errors. Hard-coding extra information in Scala code hides it from the auditing and/or translation workflows. I am not sure what's the current policy in Spark, but in Delta - for the reasons mentioned above - we are trying to gradually eradicate any user-facing hard-coded error messages in Scala. |
3 participants
What changes were proposed in this pull request?
Currently whenever an error class has at least one subclass (e.g., SPARK_ERROR_CLASS.SUBCLASS), attempting to raise an error using only the main class (SPARK_ERROR_CLASS without any subclass) hits an assertion in the error infra. If any subclass exists, a subclass has to always be used.
This change slightly alters the code that fetches the error class/subclass template to lift this limitation.
Why are the changes needed?
This limitation is very inconvenient. The behavior might be unintentional/a bug. A pattern where additional information is provided via subclass only when available is common. This change will result in cleaner error messages for the users in the long term.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Adds unit test.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code.