strongduanmu commented on PR #38702:
URL: https://github.com/apache/shardingsphere/pull/38702#issuecomment-4476377897
• Merge Verdict: Mergeable
Reviewed Scope
- PR latest head: 839d074e45e94c52f62f1c8341fa9fdb8359218e
- Changed file:
test/it/rewriter/src/test/java/org/apache/shardingsphere/test/it/rewriter/engine/SQLRewriterIT.java:103
- Related review context: SQLRewriteEngineTestParametersBuilder,
ShardingSQLRewriterIT, EncryptSQLRewriterIT, MixSQLRewriterIT, rewrite
fixtures, and scenario YAML/XML resources.
- Not Reviewed Scope: Maven/CI was not rerun per your instruction; remote
GitHub checks were unavailable from combined status.
- Need Expert Review: No.
Basis
- The root-cause path is clear: the previous implementation rebuilt
YamlRootConfiguration, ShardingSphereDatabase, RuleMetaData,
ConfigurationProperties, and ShardingSphereMetaData for every case. This PR
extracts those objects into RewriteScenarioContext and
caches them, directly addressing the slow setup path in SQLRewriteIT.
- The cache key covers the current dimensions that affect the reusable
context: test subclass, database type, rule file, case file, and
CreateTableStatement versus other SQL types. Existing mockDatabaseRules logic
only distinguishes create-table from non-
create-table in sharding/encrypt scenarios, while mix does not depend on
SQLStatement.
- Each SQL still gets parsed, hint-extracted, parameter-bound, routed, and
rewritten independently. SKIP_SQL_REWRITE and SKIP_METADATA_VALIDATE paths are
not polluted by the cache.
- Blast-radius review passed: this base test class covers sharding,
encrypt, mix, and the MySQL/PostgreSQL/Oracle/SQLServer/SQL92/openGauss
parameter dimensions. The cache key includes class and database type, so no
cross-scenario or cross-dialect reuse risk
was found.
- Adversarial pass passed: the key generator fixture is stateless; encrypt
fixtures are read-only after initialization; route/rewrite paths did not show
persistent writes to cached database/rule metadata; create-table and non-create
single-rule branches are
separated by the key.
- No SQL parser grammar, visitor, or behavior is changed, so official
syntax documentation and parser-family review are not required.
- No unrelated changes were found; the PR only changes test infrastructure.
Pre-Merge Checks
- Local CI was not rerun, based on your instruction and the PR’s stated
local pass record.
- PR metadata and diff were fetched through the GitHub connector; gh pr
view returned exit 4 because the local GitHub CLI is not authenticated, and the
connector covered the needed metadata.
- Read-only checks used: git diff --name-status/stat, rg, sed, and nl, all
exit 0; no workspace files were modified.
Self-check result: no blocking root-cause, compatibility, regression,
performance, or scope issue was found.
--
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]
