terrymanu commented on PR #39036:
URL: https://github.com/apache/shardingsphere/pull/39036#issuecomment-4903581460
### Summary
**Review Result: Mergeable**
Reason: Code-scope only. The latest PR head moves the MCP
database-type-specific behavior behind MCP capability/dialect boundaries,
preserves the previous identifier/metadata behavior through compatibility
fallbacks and focused tests, and I did not find a code-level blocker. CI not
reviewed by request.
### Evidence
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:53`
centralizes MCP dialect lookup through `TypedSPILoader`, and its fallback
behavior preserves prior empty/unknown database-type handling for quote style
and system schema filtering.
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseCapabilityOption.java:91`
adds default capability methods, so existing SPI implementations keep safe
defaults while dialect-specific option classes override only the behavior they
own.
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtils.java:85`
and `:155` now consume `MCPDatabaseDialect` for identifier case and quote
behavior instead of hard-coding database names, while `:127` and `:142` keep
DistSQL rendering fixed to back quotes.
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/metadata/jdbc/MCPJdbcMetadataLoader.java:70`
uses the dialect facade for schema semantics, system schema filtering, and
sequence query lookup, removing the previous local database-type switch.
-
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowProxyQueryService.java:145`
uses the same dialect capability for `information_schema.columns` schema
filtering.
-
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialectTest.java:43`
covers quote characters, schema semantics, identifier folding, sequence
queries, schema filtering, and unknown database fallback.
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtilsTest.java:68`
covers preserved MySQL/PostgreSQL identifier behavior and SQLServer quote
style.
### Review Details
- Review Focus: Code Correctness Review; CI not reviewed by request.
- Reviewed Scope: all 20 files reported by GitHub `/pulls/39036/files` and
local `apache/master...apache/pr/39036`, covering `mcp/core` and `mcp/support`
production/test changes. Latest PR head SHA:
`126e0322b87c2b0247afb209afd816d02fce4961`. Base and local merge-base SHA:
`3c3106913f61e9d43a94bc5e4d9f2516b05a3041`. Local triple-dot file list matched
GitHub `/pulls/{number}/files`.
- Not Reviewed Scope: GitHub Actions/check-runs, MCP E2E, and unrelated
repository areas outside the changed MCP files.
- Verification:
- `./mvnw -pl mcp/support -am -DskipITs -Dspotless.skip=true
-Dtest=MCPDatabaseDialectTest,WorkflowSQLUtilsTest,WorkflowValidationSupportTest,MCPJdbcMetadataLoaderSequenceTest,MCPJdbcMetadataLoaderSchemaFilteringTest
-Dsurefire.failIfNoSpecifiedTests=false test` on latest PR head: exit 0.
- `./mvnw -pl mcp/core -am -DskipITs -Dspotless.skip=true
-Dtest=WorkflowProxyQueryServiceTest -Dsurefire.failIfNoSpecifiedTests=false
test` on latest PR head: exit 0.
- Public PR comments and reviews were checked; no previous public review
feedback needed multi-round comparison.
- Release Note / User Docs: not required; this is an internal MCP
architecture and behavior-preservation refactor with focused tests and no
user-facing configuration, protocol, or migration 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]
