strongduanmu commented on PR #38978:
URL: https://github.com/apache/shardingsphere/pull/38978#issuecomment-4862652874
• ### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. The remaining changes are scoped refactors
in encrypt rewrite, query header building, and E2E test utilities, and I did
not find another code-level blocker in the reviewed scope. CI was not reviewed
by request.
### Evidence
- The encrypt insert-values change passes `SQLRewriteContext` into
`EncryptInsertValuesTokenGenerator` and uses it only as a fallback to recover
temporal literal text from the original SQL when the expression text is
unavailable.
- The query header refactor keeps the derived-projection path guarded by
`SelectStatementContext#containsDerivedProjections()` and falls back to JDBC
metadata for non-derived projections.
- The E2E utility changes are mostly local refactors around deterministic
map order, mounted SQL resource filtering, and native initialization helpers.
- The previous native JDBC credential replacement concern is intentionally
excluded from this review result.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #38978 latest head
`7a86c53dec1abf79caabfe00cf243fd991c6c3a0`; base and merge-base
`e4fd5c7784780b56bc8d3ead10436d94662ee326`; all 12 files from GitHub
`/pulls/38978/files`; local triple-dot file list matched GitHub.
- Not Reviewed Scope: GitHub Actions / CI logs, full E2E runtime
execution, and the native JDBC credential replacement compatibility concern
excluded by request.
- Verification: Reviewed PR metadata, file list, local triple-dot diff,
and relevant static call paths. No local Maven or E2E command was run.
- Release Note / User Docs: Not required for this internal refactor.
--
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]
