terrymanu commented on PR #38940:
URL: https://github.com/apache/shardingsphere/pull/38940#issuecomment-4816930213
### 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
introducing 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 can still use the existing conditional
formatter, so the patch does not over-apply generated-artifact quoting to
`SHOW` / `COUNT` inspection paths.
- The new algorithm utility keeps normal SPI initialization validation, and
switches `secret_reference:` cases to type-existence validation only:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowAlgorithmUtils.java:47`.
- The formatter is applied across generated rule artifacts in broadcast,
encrypt, mask, readwrite-splitting, sharding, and shadow planning services.
Examples:
-
`mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskRuleDistSQLPlanningService.java:59`
-
`mcp/features/readwrite-splitting/src/main/java/org/apache/shardingsphere/mcp/feature/readwritesplitting/tool/service/ReadwriteSplittingRuleDistSQLPlanningService.java:63`
-
`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingDistSQLPlanningService.java:228`
-
`mcp/features/shadow/src/main/java/org/apache/shardingsphere/mcp/feature/shadow/tool/service/ShadowDistSQLPlanningService.java:37`
- Sharding `DATANODES(...)` remains a string expression, not an identifier
rewrite target:
`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingDistSQLPlanningService.java:142`.
- Apply-artifact validation now covers feature-specific algorithm SPIs:
- mask: `MaskAlgorithm`,
`mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskWorkflowValidationService.java:107`
- readwrite-splitting: `LoadBalanceAlgorithm`,
`mcp/features/readwrite-splitting/src/main/java/org/apache/shardingsphere/mcp/feature/readwritesplitting/tool/service/ReadwriteSplittingRuleWorkflowValidationService.java:113`
- sharding: `ShardingAlgorithm`, `KeyGenerateAlgorithm`, and
`ShardingAuditAlgorithm`,
`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowValidationService.java:139`
- shadow: `ShadowAlgorithm`,
`mcp/features/shadow/src/main/java/org/apache/shardingsphere/mcp/feature/shadow/tool/service/ShadowWorkflowValidationService.java:120`
### Review Details
**Reviewed Scope:** Latest PR head
`3ff1c99a8c951772817f8b85287d937212156f52`, base `master` at
`50e49dd72640668a4890877e23fe37c1fec7c2b1`, merge-base
`50e49dd72640668a4890877e23fe37c1fec7c2b1`. Reviewed all 47 GitHub changed
files; local triple-dot file list matched GitHub `/pulls/38940/files`.
**Not Reviewed Scope:** I did not run a full repository `clean install`, and
I did not manually execute the generated DistSQL against a live Proxy runtime.
GitHub check-runs had no failures at review time, but two checks were still in
progress.
**Verification:**
- `./mvnw -pl
mcp/support,mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/sharding,mcp/features/shadow
-am ... test` passed, exit code `0`.
- `./mvnw -pl
mcp/support,mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/sharding,mcp/features/shadow
-am -Pcheck -DskipTests spotless:check checkstyle:check` passed, exit code `0`.
- GitHub check-runs observed for the reviewed head: 10 success, 6 skipped, 2
in progress, 0 failed.
**Release Note / User Docs:** Not required for this PR. The patch restores
generated MCP DistSQL correctness and validation behavior without changing
user-facing syntax, config keys, or migration requirements.
--
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]
