[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

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

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

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

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

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

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

...onnect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteEventsManager.scala

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

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

.../test/scala/org/apache/spark/sql/connect/pipelines/SparkDeclarativePipelinesServerTest.scala

@@ -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?

...server/src/main/scala/org/apache/spark/sql/connect/planner/StreamingForeachBatchHelper.scala

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

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

@@ -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

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+        "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

...onnect/server/src/main/scala/org/apache/spark/sql/connect/service/SessionEventsManager.scala

-        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?

garlandz-db · 2026-02-10T09:15:26Z

@heyihong thanks for the reviews. made the exceptions pass the sessionHolder key and executionHolder key where possible to populate the exception with more information

heyihong

Overall, it looks good. I left a few more comments.

heyihong · 2026-02-11T19:39:22Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+      errorClass = "SPARK_CONNECT_ILLEGAL_STATE." +
+        "STATE_CONSISTENCY_EXECUTION_STATE_TRANSITION_INVALID_OPERATION_STATUS_MISMATCH",
+      messageParameters = Map(
+        "operationId" -> executeHolder.operationId,


The function or method parameters should accept the most general abstraction. In this case, we should pass the operationId string instead of executeHolder (consider that there might be another use case that wants to reuse this code but only has access to operationId and not executeHolder).

heyihong · 2026-02-11T19:40:40Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+      errorClass = "SPARK_CONNECT_ILLEGAL_STATE." +
+        "STATE_CONSISTENCY_EXECUTION_STATE_TRANSITION_INVALID_SESSION_NOT_STARTED",
+      messageParameters = Map(
+        "sessionId" -> sessionHolder.sessionId,


Ditto. Consider passing sessionId instead of sessionHolder.

heyihong · 2026-02-11T19:42:04Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+        "EXECUTION_STATE_EXECUTE_HOLDER_ALREADY_EXISTS_GRAPH",
+      messageParameters = Map("graphId" -> graphId))
+
+  def sessionAlreadyClosed(sessionHolder: SessionHolder): SparkIllegalStateException =


Ditto. Consider passing sessionKey instead of sessionHolder.

heyihong · 2026-02-11T19:42:44Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+      errorClass = "SPARK_CONNECT_ILLEGAL_STATE.SESSION_MANAGEMENT_SESSION_ALREADY_CLOSED",
+      messageParameters = Map("key" -> sessionHolder.key.toString))
+
+  def operationOrphaned(executeHolder: ExecuteHolder): SparkIllegalStateException =


ditto. Consider passing executeKey instead of executeHolder.

heyihong · 2026-02-11T19:43:00Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+      messageParameters = Map("key" -> executeHolder.key.toString))
+
+  def sessionStateTransitionInvalid(
+      sessionHolder: SessionHolder,


heyihong · 2026-02-11T19:43:21Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+      messageParameters = Map.empty)
+
+  def streamingQueryUnexpectedReturnValue(
+      sessionHolder: SessionHolder,


heyihong · 2026-02-11T19:43:32Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+        "value" -> value.toString,
+        "context" -> context))
+
+  def cleanerAlreadySet(


heyihong · 2026-02-11T19:43:43Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+        "key" -> sessionHolder.key.toString,
+        "queryKey" -> queryKey.toString))
+
+  def eventSendAfterShutdown(sessionHolder: SessionHolder): SparkIllegalStateException =


common/utils/src/main/resources/error/error-conditions.json

heyihong · 2026-02-13T12:25:22Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+
+  def streamingQueryUnexpectedReturnValue(
+      key: String,
+      value: Any,


the type should be Int?

heyihong · 2026-02-13T12:26:57Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+
+  def cleanerAlreadySet(
+      key: String,
+      queryKey: Any): SparkIllegalStateException =


Any is too general; it should be at least a String, right?

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

garlandz-db · 2026-02-13T12:30:47Z

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala

+      errorClass = "SPARK_CONNECT_ILLEGAL_STATE.STATE_CONSISTENCY_CLEANER_ALREADY_SET",
+      messageParameters = Map(
+        "key" -> key,
+        "queryKey" -> queryKey.toString))


Suggested change

"queryKey" -> queryKey.toString))

"queryKey" -> queryKey))

...server/src/main/scala/org/apache/spark/sql/connect/planner/StreamingForeachBatchHelper.scala

heyihong

Left some final comment

common/utils/src/main/resources/error/error-conditions.json

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

sql/connect/common/src/main/scala/org/apache/spark/sql/connect/common/InvalidPlanInput.scala

@@ -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

sql/connect/common/src/main/scala/org/apache/spark/sql/connect/common/InvalidPlanInput.scala


  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

...connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala

@@ -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!

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

...t/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteResponseObserver.scala Outdated Show resolved Hide resolved

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

...nnect/server/src/main/scala/org/apache/spark/sql/connect/pipelines/PipelineEventSender.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 5, 2026

View reviewed changes

...erver/src/main/scala/org/apache/spark/sql/connect/planner/StreamingQueryListenerHelper.scala Show resolved Hide resolved

heyihong reviewed Feb 5, 2026

View reviewed changes

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/IllegalStateErrors.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 5, 2026

View reviewed changes

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SessionHolder.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 9, 2026

View reviewed changes

...erver/src/main/scala/org/apache/spark/sql/connect/planner/StreamingQueryListenerHelper.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 9, 2026

View reviewed changes

...onnect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteEventsManager.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 9, 2026

View reviewed changes

...onnect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteEventsManager.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 9, 2026

View reviewed changes

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SessionHolder.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 9, 2026

View reviewed changes

...server/src/main/scala/org/apache/spark/sql/connect/planner/StreamingForeachBatchHelper.scala Outdated Show resolved Hide resolved

garlandz-db force-pushed the SPARK-55275 branch 3 times, most recently from aa03ab3 to 3ad3fb4 Compare February 9, 2026 13:25

heyihong reviewed Feb 9, 2026

View reviewed changes

garlandz-db force-pushed the SPARK-55275 branch from 3ad3fb4 to 088ae2c Compare February 9, 2026 13:53

heyihong reviewed Feb 9, 2026

View reviewed changes

garlandz-db requested a review from heyihong February 10, 2026 08:29

garlandz-db force-pushed the SPARK-55275 branch 4 times, most recently from 28c7b12 to 684123f Compare February 10, 2026 09:12

garlandz-db changed the title ~~[SPARK-55275] SQL State Coverage: IllegalStateException~~ [SPARK-55275] SQL State Coverage: IllegalStateException AND InvalidPlanInput Feb 10, 2026

garlandz-db force-pushed the SPARK-55275 branch from 684123f to c0ef91a Compare February 10, 2026 09:18

heyihong reviewed Feb 11, 2026

View reviewed changes

garlandz-db force-pushed the SPARK-55275 branch from c0ef91a to 123d6cf Compare February 13, 2026 10:20

garlandz-db requested a review from heyihong February 13, 2026 10:23

heyihong reviewed Feb 13, 2026

View reviewed changes

garlandz-db commented Feb 13, 2026

View reviewed changes

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala Outdated Show resolved Hide resolved

garlandz-db commented Feb 13, 2026

View reviewed changes

sql/connect/server/src/main/scala/org/apache/spark/sql/connect/IllegalStateErrors.scala Outdated Show resolved Hide resolved

garlandz-db commented Feb 13, 2026

View reviewed changes

...server/src/main/scala/org/apache/spark/sql/connect/planner/StreamingForeachBatchHelper.scala Outdated Show resolved Hide resolved

heyihong reviewed Feb 13, 2026

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Show resolved Hide resolved

heyihong approved these changes Feb 13, 2026

View reviewed changes

heyihong reviewed Feb 13, 2026

View reviewed changes

garlandz-db changed the title ~~[SPARK-55275] SQL State Coverage: IllegalStateException AND InvalidPlanInput~~ [SPARK-55275] SQL State Coverage: IllegalStateException Feb 13, 2026

garlandz-db added 5 commits February 16, 2026 13:40

Add SparkILlegalStateExcpetion and InvalidPlanINput SQLSTates

91fa417

add tests

dc951f8

Apply suggestions from code review

3ec4872

update tests

dae74f6

remove invalidplaninput

c4d592d

garlandz-db force-pushed the SPARK-55275 branch from 733a7b5 to 4051ebc Compare February 16, 2026 13:40

rename exception messages back to what they were previously

8277b78

garlandz-db force-pushed the SPARK-55275 branch from 4051ebc to 8277b78 Compare February 16, 2026 16:09

Conversation

garlandz-db commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

github-actions bot commented Jan 29, 2026

JIRA Issue Information

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heyihong Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garlandz-db commented Feb 10, 2026

Uh oh!

heyihong left a comment

Choose a reason for hiding this comment

Uh oh!

heyihong Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

heyihong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

heyihong commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heyihong Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

garlandz-db commented Jan 29, 2026 •

edited

Loading

heyihong Feb 9, 2026 •

edited

Loading

heyihong Feb 11, 2026 •

edited

Loading

heyihong left a comment •

edited

Loading

heyihong commented Feb 13, 2026 •

edited

Loading

heyihong Feb 13, 2026 •

edited

Loading

heyihong Feb 13, 2026 •

edited

Loading