[j1wonpark]

What changes were proposed in this pull request?

Validate Spark Connect JDBC connections in isValid(timeout) using a lightweight Config RPC. Honor the timeout and reject negative values with SQLException.

Why are the changes needed?

The current implementation only checks local state, so it can return true after the server has closed the session.

Does this PR introduce any user-facing change?

Yes. isValid() now checks the server-side session and returns false when validation fails.

How was this patch tested?

Added seven tests. Six failed before the fix; all seven pass after it.

All 91 tests across the connection, exception mapping, client, and response validation suites passed. Scala style checks also passed.

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

Generated-by: OpenAI Codex (GPT-6)

[pan3793]

Review

The core change is sound. isValid(timeout) now performs a server round-trip through an empty Config.Get, honors positive timeouts with a gRPC deadline, and rejects negative values. Keeping isSessionValid local and adding a separate validateSession(timeout) is the right split: isSessionValid is the last-known-state latch used by isUsable and active/default session lookup, while JDBC isValid needs an active, timeout-bound probe.

Findings

• P2, requested: SparkConnectConnectionSuite only checks the healthy path (sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectConnectionSuite.scala:31). That assertion also passes with the old local-only implementation. Add an end-to-end case that calls connection.asInstanceOf[SparkConnectConnection].client.hijackServerSideSessionIdForTesting("-testing") and asserts !connection.isValid(5). Classification: coverage gap in this change.

• P3, optional: validateSession returns false for every NonFatal without logging (sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectClient.scala:188). Keep the false-on-failure behavior required by JDBC, but log unexpected exceptions at debug or warn so request-construction or response-validation bugs remain diagnosable. Classification: new code.

• P3, follow-up: INVALID_HANDLE.SESSION_CLOSED does not latch the local session state (sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ResponseValidator.scala:63). Only SESSION_CHANGED sets isSessionActive false, so after a closed-session validation, later isValid calls re-probe and checkOpen still treats the session as locally valid. Consider extending the latch to SESSION_CLOSED, or documenting why it is excluded. Classification: pre-existing-made-visible.

• P3, optional: a negative timeout passed directly to validateSession is treated as no deadline (sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/CustomSparkConnectBlockingStub.scala:132). JDBC guards this today, but the method precondition is implicit. Add a guard or document it. Classification: new code.

• P3, optional: the timeout test relies on wall-clock bounds (sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectSessionValidationSuite.scala:96). It asserts elapsed >= 500 && elapsed < 5000 around a one-second deadline; a controllable observer or a smaller timeout would reduce runtime and flakiness. Classification: new test.

• P3, documentation: clarify the two validity concepts. sql/api/src/main/scala/org/apache/spark/sql/SparkSession.scala:806 and SparkConnectClient.scala:177 can be read as promising an active check; state that isSessionValid is the last-known local state and validateSession(timeout) is the active probe. Classification: documentation gap exposed by this change.

Non-Findings

• isSessionValid should remain local; do not turn it into an RPC.
• checkOpen should remain local; only Connection.isValid(timeout) needs the active probe.
• isValid(0) using no deadline matches the JDBC contract.

Requesting the end-to-end invalidation test; the remaining items are optional follow-ups. The core design and implementation are correct.