terrymanu commented on PR #38765:
URL: https://github.com/apache/shardingsphere/pull/38765#issuecomment-4591054394
## Review Conclusion
No merge-blocking issues were found.
**Merge Verdict: Mergeable**
## Basic Information
- PR: `apache/shardingsphere#38765`
- Latest head: `b091476c8672de1c6474bd07df0779ac56d81ab7`
- Merge base: `5504a1ad85647008ed69e8dbd71e4f48fba5a685`
- Scope: 30 files
- Scope check: GitHub API file list fully matches the local triple-dot diff
## Key Review Points
### `WorkflowSQLUtils`
- DistSQL identifiers:
- Plain identifiers are emitted as-is.
- DistSQL keywords, special-character identifiers, and explicitly
delimited inputs are rendered with backticks.
- Physical SQL identifiers:
- Database keywords such as `user` and `key` are not automatically quoted.
- Only explicitly delimited identifiers or non-plain identifiers are
quoted according to the target database type.
- Safety constraints:
- NUL, line terminators, and backticks are rejected because they cannot
currently be rendered safely as reviewable identifiers.
### Workflow Planning
- PostgreSQL/openGauss:
- Unquoted identifiers are lower-folded.
- Quoted identifier semantics are preserved.
- MySQL/MariaDB/Doris:
- Identifier comparison is case-insensitive.
- Quoted and unquoted identifier semantics are not mixed.
### Encrypt / Mask
- Rule matching, derived column naming, DistSQL generation, physical DDL,
and validation SQL all use the same identifier handling logic.
- The implementation no longer spreads complex logic into parser, proxy
mainline, or e2e modules.
### Documentation
- The documentation now aligns the context clearly:
- The identifiers being handled are structured MCP tool arguments.
- MCP is not parsing SQL from natural language text.
- MCP renders reviewable DistSQL/SQL, while SQL semantics remain the
responsibility of Proxy/parser.
## Verification
- `git fetch apache master pull/38765/head:refs/remotes/apache/pr/38765`:
exit 0
- GitHub PR files vs local triple-dot diff: exit 0, fully matched
- `./mvnw -pl mcp/support,mcp/core,mcp/features/encrypt,mcp/features/mask
-am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true test`: exit 0
- `./mvnw checkstyle:check -Pcheck -T1C`: exit 0
- `git status --short --branch`: working tree clean
## Residual Risk
- Full `clean install` was not run.
- Container-level MCP e2e tests were not run.
- Given the current scope, direct unit coverage, and successful full-repo
Checkstyle run, these are not merge blockers.
--
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]
