menghaoranss commented on PR #38696:
URL: https://github.com/apache/shardingsphere/pull/38696#issuecomment-4459160192
Core conclusion: this PR is safe to merge.
**1) Decision**
- **Merge Verdict: Mergeable**
- **Reviewed Scope**
-
`database/connector/dialect/postgresql/.../PostgreSQLIdentifierCaseRuleProvider.java`
-
`database/connector/dialect/opengauss/.../OpenGaussIdentifierCaseRuleProvider.java`
- Both provider unit tests
- Schema-lookup parameterized cases in
`infra/common/.../DatabaseIdentifierContextFactoryTest.java`
- **Not Reviewed Scope**
- No full local rebuild/e2e run by me; review is based on latest diff, CI,
key runtime path, and test semantics
- **Need Expert Review**
- No
**2) Root-Cause and Fix Mapping**
- Root cause is valid: PostgreSQL/openGauss schema lookup previously
depended on lower-case matching and could miss mixed/upper stored schema names
for unquoted identifiers.
- Fix maps correctly: `IdentifierScope.SCHEMA` is overridden to insensitive
matching, while other scopes keep lower-case behavior.
- Semantics align with official docs:
- PostgreSQL: unquoted identifiers are folded to lower case; quoted
identifiers are case-sensitive.
[PostgreSQL
docs](https://www.postgresql.org/docs/current/sql-syntax-lexical.html)
- openGauss docs (`quote_ident`) describe quoting when identifier would be
case-folded, consistent with the same model.
[openGauss
docs](https://docs.opengauss.org/en/docs/latest/docs/SQLReference/character-processing-functions-and-operators.html)
**3) Risk Scan (including adversarial pass)**
- Cross-dialect/adjacent-feature regression: change is limited to
PostgreSQL/openGauss providers; MySQL/Oracle paths are untouched.
- Config-disabled/flag path: `METADATA_IDENTIFIER_CASE_SENSITIVITY` override
in `IdentifierCaseRuleResolver` remains unchanged and still has higher priority
than provider defaults.
- Partial symptom coverage risk: tests now include schema unquoted/quoted
positive/negative checks and mixed stored-case schema cases in context factory
coverage.
**4) Validation Evidence**
- CI for head `f7e691d...`: core checks are green (`CI`, `CheckStyle`,
`Spotless`, `License` all success). Some E2E jobs are skipped by matrix
strategy.
- Scope is tight: 5 files, 1 commit, no unrelated changes observed.
- Code-of-conduct alignment:
- Build/check expectations: `CODE_OF_CONDUCT.md` lines 18-20
- Test behavior expectations: lines 96-99
**5) Commands Run (exit code)**
- `curl -sL https://api.github.com/repos/apache/shardingsphere/pulls/38696`
(0)
- `curl -sL
https://api.github.com/repos/apache/shardingsphere/pulls/38696/files?per_page=100`
(0)
- `curl -sL
https://api.github.com/repos/apache/shardingsphere/pulls/38696/commits?per_page=100`
(0)
- `curl -sL
https://api.github.com/repos/apache/shardingsphere/commits/f7e691dbe0a0c0cf86e3de561a1b09cba1a9bb02/check-runs
| jq -r ...` (0)
Short summary: this is a minimal, semantics-consistent fix with adequate
risk coverage and passing CI evidence; merge is recommended.
--
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]
