You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "strongduanmu (via GitHub)" <gi...@apache.org> on 2026/05/18 02:13:56 UTC
[PR] Refactor SQLRewriterIT to test more sql rewrite case [shardingsphere]
strongduanmu opened a new pull request, #38700:
URL: https://github.com/apache/shardingsphere/pull/38700
Fixes #ISSUSE_ID.
Changes proposed in this pull request:
- Refactor SQLRewriterIT to test more sql rewrite case
---
Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
- [ ] I have updated the Release Notes of the current development version. For more details, see [Update Release Note](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/)
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor SQLRewriterIT to test more sql rewrite case [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #38700:
URL: https://github.com/apache/shardingsphere/pull/38700#issuecomment-4474033368
<img width="1622" height="1250" alt="image" src="https://github.com/user-attachments/assets/d2cf3f16-809f-4647-9b01-cd36026af41f" />
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor SQLRewriterIT to test more sql rewrite case [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu merged PR #38700:
URL: https://github.com/apache/shardingsphere/pull/38700
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor SQLRewriterIT to test more sql rewrite case [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org