terrymanu commented on PR #39322: URL: https://github.com/apache/shardingsphere/pull/39322#issuecomment-5166463758
### Result **Review Result: Mergeable** PR #39322 correctly removes the invalid assumption that any database with a default schema should be treated as PostgreSQL, while preserving PostgreSQL system-table statistics behavior for openGauss. No code-correctness blockers were found. ### Evidence - [`ShardingSphereStatisticsFactory.java:54`](https://github.com/apache/shardingsphere/blob/43b45a0fdd83556f72dda932d7d7f7a32c885cdd/infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/statistics/builder/ShardingSphereStatisticsFactory.java#L54) now resolves the SPI using the actual protocol type, preventing databases such as SQL Server, Hive, and Presto from being incorrectly treated as PostgreSQL merely because they define a default schema. - [`OpenGaussStatisticsAppender.java:27`](https://github.com/apache/shardingsphere/blob/43b45a0fdd83556f72dda932d7d7f7a32c885cdd/infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/statistics/builder/dialect/OpenGaussStatisticsAppender.java#L27) provides a dedicated openGauss SPI implementation that reuses the PostgreSQL statistics behavior. The repository’s openGauss metadata also explicitly supports `pg_catalog`, `pg_class`, and `pg_namespace`. - [`ShardingSphereStatisticsFactoryTest.java:77`](https://github.com/apache/shardingsphere/blob/43b45a0fdd83556f72dda932d7d7f7a32c885cdd/infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/statistics/builder/ShardingSphereStatisticsFactoryTest.java#L77) covers the positive openGauss path, while [`ShardingSphereStatisticsFactoryTest.java:102`](https://github.com/apache/shardingsphere/blob/43b45a0fdd83556f72dda932d7d7f7a32c885cdd/infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/statistics/builder/ShardingSphereStatisticsFactoryTest.java#L102) protects against loading the PostgreSQL appender for SQL Server. - The final JAR contains both the SPI descriptor and `OpenGaussStatisticsAppender.class`. - All local verification completed successfully: - `infra/common`: 1,096 tests, 0 failures, and 0 errors. - Module packaging, Spotless, and Checkstyle all completed with exit code 0. ### Coverage - Reviewed head: [`43b45a0fdd83`](https://github.com/apache/shardingsphere/commit/43b45a0fdd83556f72dda932d7d7f7a32c885cdd) - Merge base: `2b2ad40573ec821dd3d5cd499063f95eead9ad99` - The authoritative GitHub file list matched the local triple-dot scope; all 6 changed files were reviewed. - Behavior clusters: exact protocol-type resolution and preservation of PostgreSQL system statistics for openGauss. - The review covered root cause and behavior, blast radius and contracts, and tests and runtime, including SPI loading, database-specific distributions, singleton state, dependencies, packaging, and native-resource risks. - No unresolved evidence gaps could change the blocker set. - This is a Code Correctness Review only; CI was not queried or evaluated. -- 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]
