terrymanu commented on PR #38818: URL: https://github.com/apache/shardingsphere/pull/38818#issuecomment-4637352337
### Summary - **Merge Decision: Mergeable** - **Reason:** The latest PR head fixes the previously blocking sharding DistSQL planning issues and no unresolved root-cause, scope, or regression risk was found in the reviewed MCP workflow scope. ### Evidence - The auto-table sharding rule path now emits the sharding algorithm definition from `algorithm_type` and `algorithm_properties` instead of reusing the key-generator fragment (`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingDistSQLPlanningService.java:140`). The regression test covers `STORAGE_UNITS`, `SHARDING_COLUMN`, sharding algorithm properties, and a separate key-generate strategy (`mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingDistSQLPlanningServiceTest.java:78`). - Table-rule planning now rejects `key_generate_column` unless either `key_generator` or `key_generator_type` is available (`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningService.java:267`). The negative test verifies the clarifying response and no generated artifacts for that incomplete input (`mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningServiceTest.java:109`). - Default sharding strategy planning now validates `default_strategy_type` as `DATABASE` or `TABLE` before generating DistSQL (`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningService.java:282`). The negative test covers an invalid `FOO` value and confirms no artifacts are produced (`mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningServiceTest.java:163`). - The resulting sharding DistSQL is consistent with the existing ShardingSphere DistSQL grammar and documentation for auto-table rules and default strategies; this PR does not change parser grammar or dialect SQL parsing behavior. - The PR remains within the stated DistSQL-only workflow scope: the PR-scoped scan found no new production direct SQL execution path, migration/backfill path, physical table DDL path, storage-unit registration path, or high-frequency `ConcurrentHashMap#computeIfAbsent` path. Matches for dangerous SQL terms are limited to negative assertions or test payloads. - Shared MCP workflow support changes are generic rule-workflow plumbing for DistSQL-only artifact/guidance assembly and do not introduce target-feature semantics into Proxy/JDBC execution paths. Adjacent MCP feature modules were included in the reviewer-run module verification. ### Review Details - **Reviewed Scope:** Latest PR head `fe9006eef45b421aa513eb8b7a5e301a70bd8996`; local base ref `apache/master` at `6340fe5c90cbf90c4a2e664ec599904e76ad6867`; local merge-base `f9a8675ccd065d55d5256a595a1387b8ca6f844e`. The local triple-dot file list matched GitHub `/pulls/38818/files`: 174 files on both sides, with no path differences. Reviewed MCP support, MCP feature plugins for broadcast/readwrite-splitting/shadow/sharding, existing encrypt/mask alignment, docs, descriptors, and sharding DistSQL planning/test changes. - **Not Reviewed Scope:** GitHub Actions/check-run status was intentionally not used for the merge decision. Full `./mvnw clean install -B -T1C -Pcheck`, manual Proxy runtime smoke testing, and external MCP client/agent manual testing were not run. Parser grammar implementation was not reviewed as a changed scope because this PR does not modify parser files; grammar and docs were used only as compatibility evidence. - **Verification:** `git fetch apache master pull/38818/head:refs/remotes/apache/pr/38818` exited 0. GitHub file-list cross-check against local triple-dot diff exited 0. PR-scoped source scan for direct execution, dangerous physical operations, migration/backfill/storage-unit registration, and `computeIfAbsent` exited 0 with no production blockers. `./mvnw checkstyle:check -Pcheck -T1C` exited 0. `./mvnw -pl mcp/support,mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding -am -DskipITs -Dspotless.skip=true test` exited 0. -- 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]
