terrymanu commented on PR #39155: URL: https://github.com/apache/shardingsphere/pull/39155#issuecomment-4988993145
### Summary **Review Result: Mergeable** **Reason:** This result applies to the code scope only. The PR correctly removes MCP database query capabilities that no longer have production callers, without changing existing DistSQL query execution, result transformation, or error classification behavior. CI was not reviewed. ### Evidence - The DistSQL-only refactoring had already removed the production callers of `queryColumnDefinition`, `createProjectionValidationSql`, and `formatSQLIdentifier`. This PR removes the remaining implementations and implementation-specific tests. No references remain at the latest head. - All 40 production query calls previously passed an empty `schemaName`. Narrowing `MCPFeatureQueryFacade.query` to two parameters therefore preserves current execution behavior: `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/spi/MCPFeatureQueryFacade.java:38`. - `WorkflowProxyQueryService` still retains database-name normalization, connection management, result extraction, and JDBC error classification: `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowProxyQueryService.java:56`. - Schema switching for ordinary SQL execution remains owned by `MCPJdbcStatementExecutor` and is not removed by this PR: `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/MCPJdbcStatementExecutor.java:295`. - Query results, unavailable databases, and database error classification remain directly covered by tests: `mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowProxyQueryServiceTest.java:53`, `:104`, and `:111`. - The commits introducing `MCPFeatureQueryFacade` are not included in any release tag, and the repository contains only one implementation. Therefore, narrowing this interface does not break a released Java API. - All 28 changed files are contained within MCP modules. No protocol descriptors, configuration, dependencies, or distribution files are changed. [PR #39155](https://github.com/apache/shardingsphere/pull/39155) ### Review Details - **Review Focus:** Code Correctness Review. CI not reviewed by request. - **Reviewed Scope:** All 28 files changed at the latest head, `def550245cbfdfb5ca1cefbe17afbd106f6942f5`, across `mcp/core`, `mcp/support`, and the related feature modules. The base SHA is `958af5d721aa0c883c7a8c973e578ae46d8ee964`, and the local merge-base SHA is `812f36b9bcfe68c61d47a19d2d3f90c29e8dc414`. The local triple-dot file list exactly matched GitHub `/pulls/39155/files`. - **Not Reviewed Scope:** GitHub Actions, check runs, and real-database E2E or smoke tests. These do not affect the code-scope result. - **Verification:** `./mvnw -f mcp/pom.xml test -DskipITs -T1C`, `./mvnw spotless:apply -Pcheck -T1C`, and `./mvnw checkstyle:check -Pcheck -T1C` all completed with exit code 0 against a source tree matching the PR head. Static scans found no remaining references to the removed symbols and exactly one `MCPFeatureQueryFacade` implementation. - **Release Note / User Docs:** Not required. This PR removes unreleased internal capabilities without production callers and does not change MCP tools, resources, protocols, configuration, or user workflows. -- 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]
