terrymanu commented on PR #40300: URL: https://github.com/apache/shardingsphere/pull/40300#issuecomment-6296468411
### Result Review Result: Mergeable No code issues requiring changes were found in the latest commit. This conclusion covers code correctness only; CI was not reviewed. ### Evidence - Firebird lazily resolves and reuses the database and default schema within each SELECT/BIND description, preserving constant and parameter descriptions without a selected database. Physical columns, INSERT, and RETURNING share the same resolution boundary. [Implementation](https://github.com/apache/shardingsphere/blob/4ecb3771d13f1c1c55796f8476b569f3e00fae3b/proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/prepare/FirebirdPrepareStatementCommandExecutor.java#L236) - Three regression scenarios actually replace the schema during description processing and assert the resulting column names and types. All 25 focused Firebird tests passed, with zero failures, errors, or skipped tests. [Tests](https://github.com/apache/shardingsphere/blob/4ecb3771d13f1c1c55796f8476b569f3e00fae3b/proxy/frontend/dialect/firebird/src/test/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/prepare/FirebirdPrepareStatementCommandExecutorTest.java#L464) - JDBC preserves legitimate empty generated keys. Missing databases follow the existing exception-conversion path, original SQLExceptions are not wrapped again, and PreparedStatement fails before resource registration. - DistSQL reuses validated objects, statistics refresh continues processing subsequent tables after individual failures, and Proxy retains its finally-block cleanup. - Before reusing verification results, current source contents, configuration, tools, and complete selection scope were verified as unchanged. The remaining modified tests were reviewed through their source, fixtures, call paths, and assertions; no claim is made that the full test suite was rerun during this review. ### Coverage - Public PR #40300; HEAD `4ecb3771d13f1c1c55796f8476b569f3e00fae3b`, base `4a291dbab6a974824272b45eb04c13c66b73355e`, merge-base `3ed016a6181527342ba3b7e45466957276873683`. Local contents match the public HEAD, with no additional local changes. - The complete GitHub file inventory matches the local triple-dot comparison: 19 files and 96 changed blocks. - Requirements derive from the original constraints on null semantics, exception preservation, and minimal scope, together with existing metadata, JDBC, and Firebird contracts. The changes preserve existing capabilities without introducing APIs, SPIs, dependencies, or cross-phase atomic snapshot guarantees. - Coverage includes PostgreSQL/openGauss statistics, DistSQL export and refresh, JDBC initialization and generated keys, cluster statistics refresh, Proxy cleanup, and Firebird descriptions. Root cause and behavior, impact and contracts, and tests and runtime were examined, including compatibility, lifecycle, and performance costs. No unresolved evidence gaps remain. - Two independent review pairs and substantive verification of every assessment were completed. Every historical candidate has an explicit disposition, and the complete conclusions remained stable across both rounds. Differences in evidence premises were resolved against actual call paths and module dependencies. Session validation passed. - Local Spotless and Checkstyle verification covered all 19 files listed below. Both checks exited with code 0, with no inapplicable items. File selection, current contents, and cache records were verified. CI/Actions were not queried. Style verification command: `SPOTLESS_FILES` contains exact absolute-path regular expressions for the 19 files below, and `CHECKSTYLE_FILES` contains their corresponding source-root-relative paths. ./mvnw -B -pl infra/common,infra/distsql-handler,jdbc,mode/type/cluster/core,proxy/backend/core,proxy/frontend/dialect/firebird -DspotlessFiles="$SPOTLESS_FILES" -Dcheckstyle.includes="$CHECKSTYLE_FILES" -Dcheckstyle.includeResources=false -Dcheckstyle.includeTestResources=false spotless:check checkstyle:check -Pcheck -T1C Complete applicable file scope for both checks: - `infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/statistics/collector/postgresql/table/PostgreSQLPgClassTableStatisticsCollector.java` - `infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/statistics/collector/postgresql/table/PostgreSQLPgNamespaceTableStatisticsCollector.java` - `infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/statistics/collector/postgresql/table/PostgreSQLPgClassTableStatisticsCollectorTest.java` - `infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/statistics/collector/postgresql/table/PostgreSQLPgNamespaceTableStatisticsCollectorTest.java` - `infra/distsql-handler/src/main/java/org/apache/shardingsphere/distsql/handler/executor/configuration/export/ExportStorageNodesExecutor.java` - `infra/distsql-handler/src/main/java/org/apache/shardingsphere/distsql/handler/executor/metadata/RefreshTableMetaDataExecutor.java` - `jdbc/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSpherePreparedStatement.java` - `jdbc/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSphereStatement.java` - `jdbc/src/test/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSpherePreparedStatementTest.java` - `jdbc/src/test/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSphereStatementTest.java` - `mode/type/cluster/core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/statistics/StatisticsRefreshEngine.java` - `mode/type/cluster/core/src/test/java/org/apache/shardingsphere/mode/manager/cluster/statistics/StatisticsRefreshEngineTest.java` - `proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/ProxyJDBCExecutor.java` - `proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/ProxyJDBCExecutorTest.java` - `proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/DistSQLUpdateProxyBackendHandlerTest.java` - `proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/ral/queryable/export/ExportStorageNodesExecutorTest.java` - `proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/ral/updatable/refresh/RefreshTableMetaDataExecutorTest.java` - `proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/prepare/FirebirdPrepareStatementCommandExecutor.java` - `proxy/frontend/dialect/firebird/src/test/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/prepare/FirebirdPrepareStatementCommandExecutorTest.java` -- 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]
