menghaoranss commented on PR #38840: URL: https://github.com/apache/shardingsphere/pull/38840#issuecomment-4676433230
### Decision - **Merge Verdict: Mergeable** - **Reviewed Scope:** PR #38840 latest head `c443d00f671288f1624bf5cf63e57828c7bdcda9`, compared against local merge-base `6fb4803a72ce92bc1ddc1bce5809fd7413a12c7d`. - **Diff Verification:** GitHub `/files` returned 5 files, matching the local triple-dot diff file list. - **Compatibility Assumption:** ShardingSphere currently only preserves database/schema/table identifier case-sensitivity compatibility. Column, index, and constraint metadata lookup are expected to be case-insensitive regardless of the table-level rule. ### Review Result The change is reasonable under the current metadata compatibility boundary. `DatabaseIdentifierContextFactory` now separates storage-object scopes from database/schema/table scopes and applies an insensitive rule to: - `COLUMN` - `INDEX` - `CONSTRAINT` This is the right place to centralize the behavior because `ShardingSphereTable` builds its column, index, and constraint lookup indexes from the database identifier context. Keeping this logic in the identifier-context factory avoids restoring case-insensitive behavior separately in each metadata container. ### Positive Feedback The PR restores the previous effective behavior where column, index, and constraint lookup are case-insensitive, while keeping database/schema/table matching controlled by the database-derived or configured case rule. The added tests cover the important behavior: - column/index/constraint scopes use insensitive lookup; - database/schema/table scopes still respect the configured sensitive rule; - `ShardingSphereDatabase` verifies that table lookup remains sensitive while column lookup is insensitive. ### Non-Blocking Suggestion The behavior is subtle because database/schema/table follow one rule, while column/index/constraint intentionally follow another. A short comment near `DatabaseIdentifierContextFactory#createScopeAwareRuleSet` would make the compatibility boundary clearer for future maintainers. Suggested intent to capture: - database/schema/table case rules come from the database/configuration; - column/index/constraint metadata lookup is intentionally case-insensitive for compatibility. This is not required before merge. ### Conclusion The implementation matches the clarified compatibility target and keeps the change scoped to identifier rule construction plus focused tests. Mergeable. -- 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]
