[SPARK-55275] SQL State Coverage: IllegalStateException by garlandz-db · Pull Request #54056 · apache/spark

garlandz-db · 2026-01-29T10:55:45Z

What changes were proposed in this pull request?

Update IllegalStateException => SparkIllegalStateException in spark connect layer

Why are the changes needed?

This keeps the Spark Connect layer more traceable for errors

Does this PR introduce any user-facing change?

Yes. Changes exception type to their Spark equivalent

How was this patch tested?

Updated testing

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

Yes

github-actions · 2026-01-29T10:56:00Z

JIRA Issue Information

=== Improvement SPARK-55275 ===
Summary: add a specific SQL state for IllegalStateException errors at ExecuteEventsManager.assertStatus to ensure reliable error reporting
Assignee: None
Status: Open
Affected: ["4.0.2"]

This comment was automatically generated by GitHub Actions

heyihong · 2026-02-09T12:53:43Z

@@ -90,7 +90,7 @@ class SparkDeclarativePipelinesServerTest extends SparkConnectServerTest with St
    if (iter.hasNext) {
      iter.next()
    } else {
-      throw new IllegalStateException(s"Invalid response: $iter")
+      throw IllegalStateErrors.noBatchesAvailable()


iter is missing?

heyihong · 2026-02-09T13:48:45Z

@@ -332,7 +333,7 @@ case class SessionHolder(userId: String, sessionId: String, session: SparkSessio
    // called only once, since removing the session from SparkConnectSessionManager.sessionStore is
    // synchronized and guaranteed to happen only once.
    if (closedTimeMs.isDefined) {
-      throw new IllegalStateException(s"Session $key is already closed.")
+      throw IllegalStateErrors.sessionAlreadyClosed(sessionId)


key is missing?

heyihong · 2026-02-09T13:51:10Z

+        "value" -> value,
+        "context" -> context))
+
+  def cleanerAlreadySet(queryKey: String): SparkIllegalStateException =


I was wondering if it makes sense to have two message parameters—userId and sessionId—instead of queryKey.

the query id and run id are whats previously thrown. we can also include that

heyihong · 2026-02-09T14:01:46Z

-        sessionId: $sessionId with status ${status}
-        is not within statuses $validStatuses for event $eventStatus
-        """)
+      throw IllegalStateErrors.sessionStateTransitionInvalid(status.toString, eventStatus.toString)


sessionId and validStatuses are missing?

heyihong

Left some final comment

heyihong · 2026-02-13T13:57:00Z

Early LGTM — please fix the test failures accordingly. I also noticed a few unused subclasses (e.g., DATA_INTEGRITY_OPERATION_ID_MISMATCH, SESSION_MANAGEMENT_SERVER_SESSION_ID_CHANGED). Could you clean those up accordingly? One final thing: some error messages (e.g., “Attempting to stop the Spark Connect service that has not been started”) are being lost. Could you fix that as well?

heyihong · 2026-02-13T15:21:41Z

@@ -46,11 +46,13 @@ object InvalidPlanInput {
    InvalidPlanInput(
      errorCondition = "INTERNAL_ERROR",
      messageParameters = Map("message" -> message),
-      causeOpt = None)
+      causeOpt = None,
+      sqlState = Some("56K00"))


On second thought, it may not be a good idea to include this custom mapping in the code. It breaks the assumption that each error condition corresponds to a single SQL state. You might also need to change the error condition to a new one and store this mapping in error-conditions.json.

heyihong · 2026-02-13T15:23:32Z


  def apply(errorCondition: String, messageParameters: Map[String, String]): InvalidPlanInput =
    InvalidPlanInput(
      errorCondition = errorCondition,
      messageParameters = messageParameters,
-      causeOpt = None)
+      causeOpt = None,
+      sqlState = Some("56K00"))


heyihong · 2026-02-13T15:26:59Z

One good practice is to split a larger PR into smaller ones so that it is easier for reviewers to handle. In this case, it may be easier to review if we split the changes for IllegalStateExceptions and InvalidPlanInput into separate PRs.

heyihong · 2026-02-13T18:38:20Z

@@ -455,8 +456,7 @@ object SparkConnectService extends Logging {
    }

    if (!started) {
-      throw new IllegalStateException(
-        "Attempting to stop the Spark Connect service that has not been started.")
+      throw IllegalStateErrors.serviceNotStarted()


Friendly ping on this. It seems that some error messages (e.g., “Attempting to stop the Spark Connect service that has not been started”) are still being lost

updated good catch!

hvanhovell · 2026-02-23T18:41:10Z

    val eventSender = new PipelineEventSender(mockObserver, mockSessionHolder)
    eventSender.shutdown()
-    intercept[IllegalStateException] {
+    intercept[SparkIllegalStateException] {


You don't have to do this right? We are still throwing an IllegalStateException right?

i did it to validate the changes but yes we can change it back

hvanhovell · 2026-02-23T18:50:26Z

+import org.apache.spark.SparkFunSuite
+import org.apache.spark.sql.connect.service.{ExecuteStatus, SessionStatus}
+
+class IllegalStateErrorsSuite extends SparkFunSuite {


I am not sure I love this class. It is super mechanical, and relatively low yield. Don't we have existing tests for this, that we can tighten up?

hvanhovell

LGTM

hvanhovell · 2026-02-26T20:25:08Z

@garlandz-db can you look at the test failures?

…alStateException Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Move sqlState before subClass to match Jackson ORDER_MAP_ENTRIES_BY_KEYS alphabetical ordering required by SparkThrowableSuite. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

garlandz-db · 2026-02-26T21:50:33Z

CI is flaking. rebasing off master

github-actions Bot added SQL CONNECT labels Jan 29, 2026

garlandz-db force-pushed the SPARK-55275 branch 2 times, most recently from 6d21971 to ae01b8c Compare February 5, 2026 08:37

heyihong reviewed Feb 5, 2026

View reviewed changes

Comment thread ...t/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteResponseObserver.scala Outdated

garlandz-db force-pushed the SPARK-55275 branch from 0676a45 to 3432de2 Compare February 5, 2026 16:07

heyihong reviewed Feb 5, 2026

View reviewed changes

Comment thread ...nnect/server/src/main/scala/org/apache/spark/sql/connect/pipelines/PipelineEventSender.scala Outdated

heyihong reviewed Feb 5, 2026

View reviewed changes

Comment thread ...erver/src/main/scala/org/apache/spark/sql/connect/planner/StreamingQueryListenerHelper.scala

heyihong reviewed Feb 5, 2026

View reviewed changes

Comment thread sql/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/IllegalStateErrors.scala Outdated

heyihong reviewed Feb 5, 2026

View reviewed changes

Comment thread sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SessionHolder.scala Outdated