terrymanu commented on PR #38811:
URL: https://github.com/apache/shardingsphere/pull/38811#issuecomment-4631189007
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** PR apache/shardingsphere#38811 latest head
`243681614dd9bec8420832d4eff215fe253aa65a`; base SHA from PR API
`c3e408edf15c4262ab231d4fca73c1d27233c0cf`; local merge-base
`1e934a2f99e9ff5d9d4d2ebedda05ef8a13e852e`. Reviewed all 73 changed files under
MCP code, MCP docs, and MCP E2E tests. Local triple-dot file list matched
GitHub `/pulls/38811/files` exactly.
- **Not Reviewed Scope:** GitHub Actions / CI status; files outside the PR
diff; manual product UX beyond the automated MCP E2E coverage.
- **Need Expert Review:** No special expert review required beyond normal
MCP maintainer review.
### Basis
- The patch aligns encrypt and mask workflows with the stated strict
DistSQL-only boundary.
- Encrypt planning now uses rule-only planning context and only appends
rule DistSQL artifacts, while physical DDL, derived-column naming, index
planning, and related state classes are removed.
- Mask planning follows the same rule-only flow and no longer performs
logical metadata or SQL executability probing.
- The shared execution path keeps non-target workflows compatible.
- `WorkflowArtifactPayloadUtils.isRuleDistSQLOnlyWorkflow(...)` limits the
rule-only behavior to `encrypt.rule` and `mask.rule`.
- `WorkflowArtifactBundle.toRuleExecutableArtifacts()` and
`WorkflowExecutionService.createExecutableArtifacts(...)` ensure these two
workflow kinds execute/export only rule DistSQL artifacts.
- Generic workflows still use the existing DDL, index, and DistSQL
artifact bundle path.
- The added `MCPFeatureQueryFacade.getDatabaseType(...)` is a generic
read-only capability used for identifier matching, not an encrypt/mask-specific
semantic leak.
- `WorkflowProxyQueryService` resolves the runtime database type from the
existing capability provider.
- Encrypt and mask planning/validation now use the runtime database type
for dialect-aware identifier comparison instead of the previous empty-string
fallback.
- The linked issue #35294 is broad MCP idea/PRD tracking. This PR does not
claim to complete the whole issue; its narrower MCP encrypt/mask DistSQL-only
scope is explicit in the PR title and body.
### Verification
- Local scope boundary:
- `git diff --name-only <merge-base>..243681614dd...` produced 73 files.
- GitHub `/pulls/38811/files` produced 73 files.
- Sorted file-list diff count was 0.
- Residual behavior scan:
- No remaining `CREATE TABLE`, `ALTER TABLE`, `DROP TABLE`, `CREATE
INDEX`, `backfill`, `data cleansing`, `sql_executability`, `ddl_artifacts`,
`index_plan`, `derived_column_plan`, or `allow_index_ddl` matches in
encrypt/mask production code and descriptors.
- Reviewer-run local checks:
- `./mvnw spotless:apply -Pcheck -T1C` passed.
- `./mvnw checkstyle:check -Pcheck -T1C` passed.
- Focused encrypt/mask workflow unit tests passed.
- `./mvnw -pl test/e2e/mcp -am -Dspotless.skip=true test` passed with
`BUILD SUCCESS`.
### Notes
- I did not find blocking defects, unrelated substantive changes, or
unresolved regression risks in the latest PR version.
- One wording detail in the mask descriptor says “rule and DistSQL
artifacts”; it is not a behavioral issue because the schema and implementation
expose only rule DistSQL artifacts for this workflow.
--
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]
