j1wonpark commented on PR #57831: URL: https://github.com/apache/spark/pull/57831#issuecomment-5236040288
Thanks @nchammas — you're right. I'll drop the override and do the restructuring instead. The only use of `ErrorSubInfo.sqlState` in this PR is the cross-class case you object to, and there is no same-class case anywhere in the error JSONs that would justify the field by itself, so I'll drop it and re-scope SPARK-58619. I'll fix the README wording regardless. For the split I'd suggest `INVALID_HANDLE` keeps `FORMAT` and the `OPERATION_*` sub-conditions with `HY000`, and the three `SESSION_*` ones move to a new condition with `08003`. Two reasons beyond the error class: - `INVALID_HANDLE.FORMAT` is thrown for a malformed **session** id as well as an operation id (`SparkConnectSessionManager.validateSessionCreate` and `SparkConnectExecutionManager.ExecuteKey`), so whichever condition holds it cannot be named after operations. `INVALID_HANDLE`'s existing message stays accurate for exactly that group. - `getMessageTemplate` concatenates the condition's message with the sub-condition's, so `SESSION_CHANGED` currently reads "The handle abc is invalid. The existing Spark server driver instance has restarted." The handle is valid in that case, and keeping `SESSION_*` under `INVALID_HANDLE` would preserve that. Concretely I'd propose `SESSION_UNAVAILABLE`, with sub-conditions `CHANGED` / `CLOSED` / `NOT_FOUND` and the message "The session `<handle>` is no longer available." — every `SESSION_*` throw site already passes a session id as `handle`, so it prepends cleanly: ``` [SESSION_UNAVAILABLE.CHANGED] The session abc is no longer available. The existing Spark server driver instance has restarted. Please reconnect. SQLSTATE: 08003 ``` It avoids "handle", which is accurate only for `NOT_FOUND`; it follows the unprefixed naming of the neighbouring Connect conditions (`INVALID_HANDLE`, `INVALID_CURSOR`, `CURSOR_NOT_FOUND`, `OPERATION_CANCELED`) rather than the `CONNECT_*` form; and `_UNAVAILABLE` is already used in the file. Note it would be the first condition in class `08`. The rename is visible on the wire. These sites match the condition names as strings, and the first four also exist in already-released clients: | site | matches | effect if it stops matching | |---|---|---| | `sql/connect/common/.../client/ResponseValidator.scala:70` | `[INVALID_HANDLE.SESSION_CHANGED]` in the message text | silent: the session is never marked stale, so `SparkSession.isUsable` keeps returning true and the builder hands back a dead session | | `python/pyspark/sql/connect/client/core.py:2418` | `errorClass` metadata equal to the same name | the same, via `_closed` | | `sql/connect/common/.../client/ExecutePlanResponseReattachableIterator.scala:253-254` | `OPERATION_NOT_FOUND` **and** `SESSION_NOT_FOUND` | reattach loses one arm — under either direction, since both names are in one expression | | `python/pyspark/sql/connect/client/reattach.py:281-282` | the same two | the same | | `sql/connect/client/jdbc/.../util/JdbcErrorUtils.scala:69` | prefix `INVALID_HANDLE.SESSION_` | unreleased (SPARK-57933), so just an in-tree edit | The two reattach matchers are already a `||` or a list, so accepting the new name alongside the old is one line each. I'd do that much and no more — we have renamed released conditions without a migration-guide entry before, SPARK-48176 being a recent example. Happy to implement this here if you agree with the shape. Separately, the PySpark commit in this PR is unrelated to the above — `python/pyspark/errors/error-conditions.json` does not contain `INVALID_HANDLE` at all. It makes `get_sqlstate` fall back to the main condition's SQLSTATE, fixing cases such as `NEAREST_BY_JOIN.UNSUPPORTED_MODE` reporting none. I'll move it to its own PR either way. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
