menghaoranss commented on PR #39246: URL: https://github.com/apache/shardingsphere/pull/39246#issuecomment-5099066257
```markdown ### Summary **Review Result: Mergeable** **Reason:** The change addresses the root cause, and the storage identifier policy is propagated through every derived-column-name path. No code-level blocker was found. This result covers code scope only; CI not reviewed by request. ### Evidence - `EncryptDerivedColumnSuffix#getDerivedColumnName` no longer uses the deprecated `DatabaseTypeRegistry#formatIdentifierPattern`. It now calls `DatabaseIdentifierContext.normalizeStorage(IdentifierScope.COLUMN, ...)`, which matches the fact that derived identifiers are emitted to storage: `features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/enums/EncryptDerivedColumnSuffix.java:45`. - `EncryptTokenGenerateBuilder` obtains the identifier context from the current `ShardingSphereDatabase` and propagates it through the projection, predicate, and group-by generators. The assignment path reuses its existing database reference: `features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/rewrite/token/EncryptTokenGenerateBuilder.java:66`. - Repository-wide call-site inspection confirms that all projection, predicate, group-by, and assignment calls were migrated. No `formatIdentifierPattern` call remains in encrypt core. - `DatabaseIdentifierContext` owns refreshable storage policies. Retaining the context reference does not create a stale policy snapshot. - The new focused unit test verifies derived-suffix normalization with a lower-case storage policy, while the existing generator tests cover constructor propagation and prior rewrite behavior: `features/encrypt/core/src/test/java/org/apache/shardingsphere/encrypt/enums/EncryptDerivedColumnSuffixTest.java:29`. - The high-frequency paths introduce no blocking I/O, hidden shared state, or `computeIfAbsent`. They also avoid constructing a `DatabaseTypeRegistry` for each derived name. - The PR does not change configuration, SPI contracts, SQL semantics, dependencies, packaging, or diagnostics. No user-facing migration requirement was found. ### Review Details - **Review Focus:** Code Correctness Review. CI not reviewed by request. - **Reviewed Scope:** All 13 PR files in `features/encrypt/core`, comprising 9 production files and 4 test files. Latest PR head: `168789c8d3d15942014f3e73044423a76f590bc7`; base and local merge-base: `12a2b1109d8a990c714e03fd00db7f4a18b1a95f`. The local triple-dot file list exactly matched GitHub `/pulls/39246/files`. - **Not Reviewed Scope:** GitHub Actions, check-runs, and workflow logs were not reviewed. Modules outside the PR scope were inspected only to confirm identifier-policy ownership and lifecycle. - **Verification:** `./mvnw spotless:check -Pcheck -T1C` exited with `0`; `./mvnw checkstyle:check -Pcheck -T1C` exited with `0` and reported no violations; focused encrypt core tests exited with `0`, with 14 tests, 0 failures, and 0 errors. - **Release Note / User Docs:** Not required. This is an internal identifier-normalization dependency migration in encrypt core and introduces no configuration, protocol, operational, or migration requirements. ``` -- 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]
