terrymanu commented on PR #39037: URL: https://github.com/apache/shardingsphere/pull/39037#issuecomment-4904470332
### Summary **Review Result: Mergeable** Reason: This is a code-scope only result; CI was not reviewed by request. The PR removes the legacy centralized system-schema fallback and makes system schema recognition depend only on the database capability option, with focused coverage for the intended behavior change and no code-level blocker found in the reviewed scope. ### Evidence - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:120` now resolves system schemas only through `MCPDatabaseCapabilityOption#getSystemSchemas`, so unknown database types no longer inherit centralized fallback schemas. - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:137` keeps the catalog-based `DATABASE_AS_SCHEMA` path delegated to the same `isSystemSchema` predicate, so the existing schema/catalog semantics remain consistent. - `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialectTest.java:84`, `:90`, `:102`, and `:108` cover explicit known-dialect positives, catalog semantics, unknown database false behavior, and cross-dialect isolation. - Static scope checks found no remaining `LEGACY_SYSTEM_SCHEMAS` fallback in the reviewed MCP support production/test paths. ### Review Details - Review Focus: Code Correctness Review; CI not reviewed by request. - Reviewed Scope: PR #39037 latest head `fe65f2af9c7cec6310a192f6cf5cf7fff607f871`, base/merge-base `ea9c85c26a3b6773b574f6f5e8ac8a44dceae0ce`; reviewed all GitHub-listed changed files, and the local triple-dot file list matched GitHub `/pulls/39037/files`. - Not Reviewed Scope: GitHub Actions/check-runs, workflow logs, broader MCP/E2E runtime behavior outside the two changed files, and unrelated repository areas. - Verification: `./mvnw -pl mcp/support -am -DskipITs -Dspotless.skip=true -Dtest=MCPDatabaseDialectTest,MCPJdbcMetadataLoaderSchemaFilteringTest,MCPJdbcMetadataLoaderSequenceTest -Dsurefire.failIfNoSpecifiedTests=false test` passed with exit code 0 on the latest PR head. - Release Note / User Docs: Not required for this PR because it is an internal MCP dialect capability cleanup with targeted behavior tests and no configuration key, public API, dependency, packaging, or user-facing documentation surface changed. -- 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]
