terrymanu commented on PR #39057: URL: https://github.com/apache/shardingsphere/pull/39057#issuecomment-4914524641
### Summary **Review Result: Mergeable** Reason: This result covers only Code Correctness Review scope. The latest PR code removes an unused MCP `information_schema` column lookup facade method, its only implementation, the related dialect option, and matching tests. I did not find any code-level blocker. CI not reviewed by request. ### Evidence - PR #39057 latest head is `85fb5a603e5796eb86ad18d3f0204be41a88eb9a`; base is `17410a4ef772221472dcab062e676227d158c4c0`; changed scope is 10 MCP files, `+0/-163`. - The GitHub `/pulls/39057/files` file list matches the local triple-dot diff `17410a4ef772221472dcab062e676227d158c4c0...85fb5a603e5796eb86ad18d3f0204be41a88eb9a`. - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/spi/MCPFeatureQueryFacade.java` removes `queryInformationSchemaColumnNames`; `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowProxyQueryService.java` removes the only implementation of that method and its private SQL construction logic. - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseCapabilityOption.java` and `MCPDatabaseDialect.java` remove the `isInformationSchemaColumnSchemaFilterRequired` capability that was used only by that dead path; the matching MySQL, MariaDB, PostgreSQL, and openGauss option overrides are also removed. - An exact scan on the PR head for `queryInformationSchemaColumnNames`, `isInformationSchemaColumnSchemaFilterRequired`, `createInformationSchemaColumnQuery`, and `shouldFilterBySchema` found no remaining references. - On the PR head, `WorkflowProxyQueryService` remains the only concrete implementation of `MCPFeatureQueryFacade`; MCP reactor tests cover compilation after the interface method removal and the existing MCP unit tests. ### Review Details - Review Focus: Code Correctness Review. CI not reviewed by request. - Reviewed Scope: PR #39057 latest head `85fb5a603e5796eb86ad18d3f0204be41a88eb9a`; base `master` SHA `17410a4ef772221472dcab062e676227d158c4c0`; local merge-base `17410a4ef772221472dcab062e676227d158c4c0`; reviewed all 10 changed files under `mcp/core` and `mcp/support`; local file list matched GitHub `/pulls/39057/files`. - Not Reviewed Scope: GitHub Actions/check-runs, Docker-based MCP E2E runtime behavior, and non-MCP modules beyond reference-scan impact. - Verification: `./mvnw -f mcp/pom.xml -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Drat.skip=true test -Pcheck` exited `0`; exact deleted-symbol scan on the PR head found no remaining references. - Release Note / User Docs: Not required; this is internal MCP API cleanup with no user-facing configuration, syntax, migration, or operational behavior change. -- 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]
