terrymanu commented on PR #39147: URL: https://github.com/apache/shardingsphere/pull/39147#issuecomment-4988723088
### Summary **Review Result: Mergeable** Reason: Code-scope review found no blocking correctness, compatibility, or regression issue in the latest head; CI not reviewed by request. ### Evidence - Reviewed latest head `1540ae3f8106931841a3f098ec392e6378c24507` against merge-base `cb50781bc2a08ba20d7b9a66ec5e017fc2bf17c5`. - GitHub’s authoritative 62-file PR scope exactly matched the local triple-dot diff. - All 21 production/resource files and 41 test/build files were reviewed. - The JDBC metadata change consistently separates relation discovery from request-scoped column/index loading, preserves schema semantics, escapes metadata patterns, orders columns by ordinal position, and reconstructs composite-index metadata. - Metadata payload consumers, completion, search, workflow validation, and the `MCPMetadataQueryFacade` contract were updated consistently. - Workflow output schemas now require the stable fields actually emitted by `WorkflowPlanPayloadBuilder`. - Default Shadow algorithm planning now consistently enforces and emits `SQL_HINT`, matching the existing ShardingSphere Shadow configuration constraint. - Transport timeout and notification behavior are covered for both STDIO and Streamable HTTP paths. - Focused MCP verification passed: 287 tests, 0 failures, 0 errors. - MCP E2E test suite passed: 471 tests, 0 failures, 0 errors, 106 environment-gated tests skipped. - MCP E2E test compilation, scoped Spotless checks, and scoped Checkstyle checks passed. - A final adversarial pass over JDBC boundaries, cache ownership, model-facing schemas, Shadow DistSQL, transport notifications, and E2E lifecycle found no additional independent issue. ### Review Details - Reviewed Scope: all production code, descriptors, unit tests, E2E support, runtime tests, build configuration, and fixtures changed by PR #39147. - Not Reviewed Scope: Docker-backed production E2E execution and external database behavior beyond the included deterministic tests. - Verification: focused module tests, MCP-E2E unit tests, E2E test compilation, Spotless, and Checkstyle. - Release Note/User Docs: no separate update is required; model-facing schema descriptions were updated in the patch, metadata fields are additive, and the Shadow restriction reflects an existing runtime contract. - CI not reviewed by request. -- 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]
