terrymanu commented on PR #39083: URL: https://github.com/apache/shardingsphere/pull/39083#issuecomment-4955389500
### Summary **Review Result: Mergeable** Result: No code issues were found that should block merging. Reason: The PR loads and propagates identifier case policies at the correct boundary, preserves qualification and quoting semantics during cross-schema validation, and provides sufficient syntax and JDBC result-set evidence for enabling plain `EXPLAIN` on ClickHouse and Hive. ### Evidence - `MCPJdbcDatabaseProfileLoader.java:69` loads policies through the main project’s `IdentifierCasePolicyResolver` and stores them in `RuntimeDatabaseProfile`; `MCPDatabaseCapabilityProvider.java:61` only assembles capabilities. - `SQLStatementObjectName.java:25`, `SQLStatementObjectNameReader.java:50`, and `SQLStatementTargetResolver.java:46` preserve the first identifier, quote character, and qualification state. - `MCPSQLExecutionFacade.java:162` uses the schema-level `IdentifierCasePolicy` to distinguish current-database references from cross-schema references. Case-sensitive, case-insensitive, and quoted scenarios are covered by tests. - `ExplainSQLCandidateValidator.java:40` continues to reject `EXPLAIN ANALYZE`, rewritten SQL, executable comments, and output redirection. - Plain `EXPLAIN` syntax is supported by the [ClickHouse documentation](https://clickhouse.com/docs/sql-reference/statements/explain) and [Hive documentation](https://hive.apache.org/docs/latest/language/languagemanual-explain/). The pinned JDBC versions and their result-set execution paths were also verified. - PR head `adbd8437f04a502a88bde222488f2dd3a322e1d7` contains 30 changed files. The local file list exactly matches GitHub’s authoritative file list, and every changed file is under `mcp/**`. ### Review Details - Review Focus: Code Correctness Review. CI not reviewed by request. - Reviewed Scope: 14 production files and 16 test files, covering identifier policy ownership, cross-schema protection, EXPLAIN safety, compatibility, dependencies, and adjacent call paths. - Verification: 277 focused tests passed. Spotless and Checkstyle passed for all affected modules. - Not Reviewed Scope: No live ClickHouse or Hive database smoke test was run. Official syntax and the pinned JDBC-driver execution paths provide deterministic supporting evidence. - Documentation: No configuration format, MCP tool protocol, or public workflow changed, so no user documentation or release note is required. - Workspace: The review did not modify code. Two pre-existing untracked `__pycache__` directories were left untouched. -- 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]
