Skip to content

Commit 37f8df4

Browse files
imarkowitzcloud-fan
andcommitted
[SPARK-53507][CORE] Don't use case class for BreakingChangeInfo
### What changes were proposed in this pull request? Don't use a Scala case class for BreakingChangeInfo and MitigationConfig ### Why are the changes needed? Per comment: #52256 (comment) > [cloud-fan](https://github.com/cloud-fan) [5 days ago](#52256 (comment)) > I just realize that we expose it as a public API via SparkThrowable.getBreakingChangeInfo. We shouldn't expose a case class as public API as it has a wide API surface, including the companion object. > We should follow SparkThrowable and define it in Java. ### Does this PR introduce _any_ user-facing change? No -- interface is almost the same ### How was this patch tested? Updated unit tests ### Was this patch authored or co-authored using generative AI tooling? Used Github co-pilot, mainly for the `equals` and `hashCode` functions Closes #52484 from imarkowitz/ian/breaking-changes-case-class. Lead-authored-by: imarkowitz <ian.markowitz@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
1 parent ce3437a commit 37f8df4

File tree

3 files changed

+44
-14
lines changed

3 files changed

+44
-14
lines changed

common/utils/src/main/scala/org/apache/spark/ErrorClassesJSONReader.scala

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -203,18 +203,48 @@ private case class ErrorSubInfo(
203203
* If false, the spark job should be retried by setting the
204204
* mitigationConfig.
205205
*/
206-
case class BreakingChangeInfo(
207-
migrationMessage: Seq[String],
208-
mitigationConfig: Option[MitigationConfig] = None,
209-
needsAudit: Boolean = true
210-
)
206+
class BreakingChangeInfo(
207+
val migrationMessage: Seq[String],
208+
val mitigationConfig: Option[MitigationConfig] = None,
209+
val needsAudit: Boolean = true) {
210+
override def equals(other: Any): Boolean = other match {
211+
case that: BreakingChangeInfo =>
212+
migrationMessage == that.migrationMessage &&
213+
mitigationConfig == that.mitigationConfig &&
214+
needsAudit == that.needsAudit
215+
case _ => false
216+
}
217+
218+
override def hashCode(): Int = {
219+
val prime = 31
220+
var result = 1
221+
result = prime * result + migrationMessage.hashCode()
222+
result = prime * result + mitigationConfig.hashCode()
223+
result = prime * result + needsAudit.hashCode()
224+
result
225+
}
226+
}
211227

212228
/**
213229
* A spark config flag that can be used to mitigate a breaking change.
214230
* @param key The spark config key.
215231
* @param value The spark config value that mitigates the breaking change.
216232
*/
217-
case class MitigationConfig(key: String, value: String)
233+
class MitigationConfig(val key: String, val value: String) {
234+
override def equals(other: Any): Boolean = other match {
235+
case that: MitigationConfig =>
236+
key == that.key && value == that.value
237+
case _ => false
238+
}
239+
240+
override def hashCode(): Int = {
241+
val prime = 31
242+
var result = 1
243+
result = prime * result + key.hashCode()
244+
result = prime * result + value.hashCode()
245+
result
246+
}
247+
}
218248

219249
/**
220250
* Information associated with an error state / SQLSTATE.

core/src/test/scala/org/apache/spark/SparkThrowableSuite.scala

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -570,10 +570,10 @@ class SparkThrowableSuite extends SparkFunSuite {
570570
val breakingChangeInfo = reader.getBreakingChangeInfo("TEST_ERROR")
571571
assert(
572572
breakingChangeInfo.contains(
573-
BreakingChangeInfo(
573+
new BreakingChangeInfo(
574574
Seq("Migration message with <param2>."),
575-
Some(MitigationConfig("config.key1", "config.value1")),
576-
needsAudit = false)))
575+
Some(new MitigationConfig("config.key1", "config.value1")),
576+
false)))
577577
val errorMessage2 =
578578
reader.getErrorMessage("TEST_ERROR_WITH_SUBCLASS.SUBCLASS", error2Params)
579579
assert(
@@ -582,9 +582,9 @@ class SparkThrowableSuite extends SparkFunSuite {
582582
val breakingChangeInfo2 = reader.getBreakingChangeInfo("TEST_ERROR_WITH_SUBCLASS.SUBCLASS")
583583
assert(
584584
breakingChangeInfo2.contains(
585-
BreakingChangeInfo(
585+
new BreakingChangeInfo(
586586
Seq("Subclass migration message with <param3>."),
587-
Some(MitigationConfig("config.key2", "config.value2")))))
587+
Some(new MitigationConfig("config.key2", "config.value2")))))
588588
}
589589
}
590590

sql/connect/server/src/test/scala/org/apache/spark/sql/connect/service/FetchErrorDetailsHandlerSuite.scala

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -291,9 +291,9 @@ class FetchErrorDetailsHandlerSuite extends SharedSparkSession with ResourceHelp
291291
val migrationMessages =
292292
Seq("Please update your code to use new API", "See documentation for details")
293293
val mitigationConfig =
294-
Some(MitigationConfig("spark.sql.legacy.behavior.enabled", "true"))
294+
Some(new MitigationConfig("spark.sql.legacy.behavior.enabled", "true"))
295295
val breakingChangeInfo =
296-
BreakingChangeInfo(migrationMessages, mitigationConfig, needsAudit = false)
296+
new BreakingChangeInfo(migrationMessages, mitigationConfig, false)
297297

298298
val testError = new TestSparkThrowableWithBreakingChange(
299299
"TEST_BREAKING_CHANGE_ERROR",
@@ -342,7 +342,7 @@ class FetchErrorDetailsHandlerSuite extends SharedSparkSession with ResourceHelp
342342
test(
343343
"throwableToFetchErrorDetailsResponse with breaking change info without mitigation config") {
344344
val migrationMessages = Seq("Migration message only")
345-
val breakingChangeInfo = BreakingChangeInfo(migrationMessages, None, needsAudit = true)
345+
val breakingChangeInfo = new BreakingChangeInfo(migrationMessages, None, true)
346346

347347
val testError = new TestSparkThrowableWithBreakingChange(
348348
"TEST_BREAKING_CHANGE_NO_MITIGATION",

0 commit comments

Comments
 (0)