strongduanmu commented on PR #38700:
URL: https://github.com/apache/shardingsphere/pull/38700#issuecomment-4473940620
• Merge Verdict: Mergeable
This is a code review result. The latest head is
fb14077fac4ca01907ce76c3e54b165b6bc76203. GitHub checks are still queued, so CI
should finish before merge.
Reviewed Scope
- PR: apache/shardingsphere#38700
- Base: master
- Head: fb14077fac4c
- Files changed: 9 files, +426/-35
- Reviewed areas:
- SQLRewriterIT test framework refactor
- SQLRewriteEngineTestParametersBuilder parameter construction logic
- DialectStorageUnitMetaDataMocker test SPI
- MySQLStorageUnitMetaDataMocker MySQL dialect mock
- JAXB entity attributes
- SPI service registration file
- AGENTS.md changes
Findings
No merge-blocking issues remain within the reviewed scope after the
explicit waivers.
The following 3 items are intentionally waived:
- SQLRewriteEngineTestParametersBuilder.java:179-198 and 287-290 generate
literal SQL and literal expected SQL through string replacement.
- The PR title says “test more sql rewrite case”, while this PR does not
add actual rewrite cases.
- AGENTS.md:35 and 191-192 are workflow documentation changes outside the
test refactor scope.
Code Review Details
- SQLRewriterIT.java:151-187
The refactor separates ShardingSphereDatabase creation,
ConfigurationProperties creation, and StorageUnit mocking. The structure is
more extensible for the test framework.
createStorageUnits loads dialect metadata mockers through
DatabaseTypedSPILoader.findService(DialectStorageUnitMetaDataMocker.class,
databaseType), which is suitable for future PostgreSQL, Oracle, openGauss, and
other dialect-specific test behavior.
- SQLRewriterIT.java:164-168
The default METADATA_IDENTIFIER_CASE_SENSITIVITY=INSENSITIVE is
acceptable for this IT setup. It stabilizes mocked metadata identifier case
behavior.
- DialectStorageUnitMetaDataMocker.java
The SPI is scoped to the test module and has a clear responsibility:
mocking storage unit metadata.
It stays under test/it/rewriter and does not affect production code.
- MySQLStorageUnitMetaDataMocker.java:49-55
MySQL lower_case_table_names mocking is isolated from SQLRewriterIT,
keeping dialect-specific details out of the base IT class.
- MySQLStorageUnitMetaDataMocker.java:58-103
The DatabaseMetaData#getTypeInfo() mock covers common MySQL types and
supports rewrite/binder scenarios that depend on type metadata. As test fixture
logic, the risk is acceptable.
- SQLRewriteEngineTestParametersBuilder.java:131-153
Case-level yaml-rule and ${databaseType} / ${databaseTypeLower}
placeholders make future multi-database cases easier to reuse.
- RewriteAssertionEntity.java, RewriteInputEntity.java,
RewriteOutputEntity.java
The new yaml-rule and sql-file JAXB attributes are simple extensions and
remain compatible with existing XML cases.
- META-INF/services/...DialectStorageUnitMetaDataMocker
The SPI registers MySQLStorageUnitMetaDataMocker. The ShardingSphere
side currently introduces only the MySQL dialect mock, which keeps the scope
reasonable.
CI / Verification
- Latest PR API status:
- mergeable=true
- mergeable_state=unstable
- Reason: latest head checks are still queued
- Current check-runs:
- Import Global Environment / init-environment: queued
- Detect Changed Files: queued
- Detect Changes and Generate Matrix: queued
- Maven IT was not run locally; this review was read-only.
Not Reviewed Scope
- The corresponding dbplusengine mirror changes were not reviewed.
- test/it/rewriter IT was not run locally.
- Future Oracle/PostgreSQL fixture quality was not reviewed because this
PR does not add those cases.
Pre-Merge Checks
Before merging, confirm that the latest GitHub checks complete
successfully.
Recommended local commands if maintainers require local verification:
./mvnw -pl test/it/rewriter -DskipITs -Dspotless.skip=true
'-Dtest=*SQLRewriterIT' -Dsurefire.failIfNoSpecifiedTests=false test
./mvnw spotless:apply -Pcheck -T1C
./mvnw checkstyle:check -Pcheck -T1C
--
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]
