terrymanu commented on PR #38940:
URL: https://github.com/apache/shardingsphere/pull/38940#issuecomment-4817507468
### Summary
**Review Result: Mergeable**
**Reason:** The latest PR head fixes the generated MCP rule DistSQL quoting
path generically and adds cross-feature algorithm SPI validation without a
confirmed blocker in the reviewed scope.
### Evidence
- Generated rule DistSQL now uses a dedicated generated-artifact identifier
formatter that always back-quotes non-empty identifiers:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtils.java:146`.
- Regular inspection/query DistSQL still uses the existing conditional
formatter, so `SHOW` / `COUNT` inspection paths are not over-changed.
- Algorithm validation now goes through a shared Typed SPI availability
utility. Normal properties still use `TypedSPILoader.checkService`, while
`secret_reference:` properties only require the algorithm type to exist:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowAlgorithmUtils.java:47`.
- Generated rule quoting is applied across encrypt, mask,
readwrite-splitting, sharding, broadcast, and shadow planners. For sharding,
`DATANODES('...')` remains a string literal while generated identifiers go
through the new formatter:
`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingDistSQLPlanningService.java:43`
and
`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingDistSQLPlanningService.java:228`.
- The previous MCP E2E expectation mismatch is covered in the latest head:
encrypt expectations now assert quoted cipher / assisted-query / drop SQL, and
mask expectations now assert quoted create/drop SQL:
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/HttpProductionProxyEncryptWorkflowE2ETest.java:96`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/HttpProductionProxyEncryptWorkflowE2ETest.java:149`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/HttpProductionProxyEncryptWorkflowE2ETest.java:293`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/HttpProductionProxyMaskWorkflowE2ETest.java:75`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/HttpProductionProxyMaskWorkflowE2ETest.java:99`
### Review Details
- **Reviewed Scope:** Latest PR head
`18acba1e59ae120c7d75ceed247f557acd153663`, PR base SHA
`50e49dd72640668a4890877e23fe37c1fec7c2b1`, local merge-base
`50e49dd72640668a4890877e23fe37c1fec7c2b1`. GitHub `/pulls/38940/files` and
local triple-dot scope both contain 49 files and match. Reviewed MCP support,
broadcast, encrypt, mask, readwrite-splitting, sharding, shadow, descriptors,
unit tests, and MCP E2E expectation updates.
- **Not Reviewed Scope:** Areas outside the PR changed-file scope were not
reviewed in detail. No SQL parser, database protocol, transaction,
dependency-license, or distribution packaging behavior change was identified
beyond MCP module dependencies needed by the changed code.
- **Verification:** Current public check-runs on head
`18acba1e59ae120c7d75ceed247f557acd153663` show success for `CI`, `Check -
CheckStyle`, `Check - License`, `Check - Spotless`, `MCP - Unit Tests with JDK
21`, and `E2E - MCP MySQL Runtime`. I also reviewed the full 49-file diff scope
and reran the root-cause, adjacent-feature, constructor/API, descriptor, and
regression-risk checks for this head.
- **Release Note / User Docs:** No release note is required for this PR
because it fixes MCP generated artifact correctness and validation behavior
inside the existing MCP workflow surface. MCP descriptors and examples affected
by the generated SQL contract are updated in the PR.
--
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]
