terrymanu commented on PR #39041: URL: https://github.com/apache/shardingsphere/pull/39041#issuecomment-4906219448
### Summary **Review Result: Mergeable** Reason: No blocking issue was found in the reviewed code scope. This result is code-scope only, CI not reviewed by request, and does not represent the final repository merge gate. ### Evidence - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:63` obtains connector dialect metadata through the `DatabaseType` SPI, avoiding database-specific dispatch inside MCP. When the connector SPI is absent, it still falls back to the MCP capability option, matching the PR’s connector-absent fallback model. - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:83` reuses `DialectDatabaseMetaData#getQuoteCharacter()` while preserving the existing MCP option and unknown-type fallback behavior. - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:102` and `:132` reuse the main-project identifier case policy only for connector metadata with the `LOWER_CASE` identifier pattern, avoiding behavior changes for `KEEP_ORIGIN` databases such as MySQL, MariaDB, and SQLServer. - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:184` reuses connector system schemas only when the connector list is empty or equivalent to the MCP option list; otherwise it preserves the MCP option. This covers compatibility paths such as openGauss and Firebird where connector and MCP option system schemas are not equivalent. - `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialectTest.java:63`, `:84`, `:113`, `:152`, `:166`, and `:182` cover the key precedence paths for quote character, lower-case identifiers, folded identifiers, connector system schemas, option fallback, and ignoring non-equivalent connector schemas. ### Review Details - Review Focus: Code Correctness Review; CI not reviewed by request. - Reviewed Scope: PR #39041 latest head `0b0f97474ec3b3b604c7c51b168bbfc649a15168`, base `master` at `96d71df59a1c0a31601778a384290f129acc57b2`, local merge-base `96d71df59a1c0a31601778a384290f129acc57b2`; local triple-dot file list matched GitHub `/pulls/39041/files`. - Reviewed Files: `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java`, `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialectTest.java`. - Not Reviewed Scope: GitHub Actions/check-runs/workflow logs, MCP E2E/runtime distribution smoke, and unrelated modules outside the direct connector metadata contracts. - Verification: `./mvnw -pl mcp/support -DskipITs -Dspotless.skip=true test` exited 0; `./mvnw -pl mcp/support spotless:check -Pcheck -T1C` exited 0; `./mvnw -pl mcp/support checkstyle:check -Pcheck -T1C` exited 0. - Release Note / User Docs: Not required; this is an internal MCP capability-source refactor with compatibility fallbacks and no user-facing configuration, syntax, or diagnostic 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]
