terrymanu commented on PR #39141: URL: https://github.com/apache/shardingsphere/pull/39141#issuecomment-4985029005
### Summary **Review Result: Mergeable** **Reason:** The latest code correctly centralizes JDBC exception classification and replaces SQL text heuristics with parsed object metadata. No code-level blockers were found. This conclusion covers code correctness only; CI was not reviewed by request. ### Evidence - `MCPJDBCExceptionClassifier` evaluates standard JDBC evidence before using the dialect SPI to resolve ambiguity. It correctly handles causes, next exceptions, cycles, and explicitly classified wrapper exceptions. - MySQL and MariaDB authorization, object visibility, and syntax errors are no longer incorrectly classified solely because of `SQLSyntaxErrorException`. - `MCPJdbcStatementExecutor` and `WorkflowProxyQueryService` classify failures while the database type is still available, then propagate the category to the error recovery layer. - `SQLStatementObjectExtractor` explicitly marks database, schema, and DCL namespace targets. `MCPSQLExecutionFacade` no longer relies on SQL text scanning. - Suppressed exceptions do not override the primary SQL failure, while a concrete error in `SQLException#getNextException()` can override a conservative dialect fallback. - `hive-jdbc` is test-scoped in MCP E2E and does not affect production or distribution dependencies. - Tests cover ambiguous MySQL `42000` errors, authorization error codes, exception chains, DCL namespaces, cross-schema checks, and safe error responses. ### Review Details - **Review Focus:** Code Correctness Review. CI not reviewed by request. - **Reviewed Scope:** All 30 files in the PR across `mcp/core`, `mcp/support`, and `test/e2e/mcp`. Latest head: `8b3197261a5304a322c080186d2f09195718273a`; merge base: `e249deba59d3583dc5464292672c5c73a92c2832`. The local file list exactly matched the GitHub PR file list. - **Not Reviewed Scope:** GitHub Actions, check runs, and independent validation against live databases. - **Verification:** All 592 MCP Support tests and 680 MCP Core tests passed. Checkstyle passed. The Hive test dependency tree for MCP E2E was also verified successfully. - **Release Note / User Docs:** Not required. This PR introduces no configuration changes, migration steps, or new user-facing operational contracts. -- 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]
