terrymanu commented on PR #39232: URL: https://github.com/apache/shardingsphere/pull/39232#issuecomment-5066809655
### Summary **Review Result: Mergeable** Reason: This result covers code scope only; CI not reviewed by request. The PR precisely removes obsolete TODOs and a behaviorally redundant branch without changing public APIs, SPIs, configuration, dependencies, or runtime behavior. ### Evidence - In [FirebirdAdminExecutorCreator.java:38-46](https://github.com/apache/shardingsphere/blob/b3c8b5549413153907d5da4e5f732452a270c308/proxy/backend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/backend/firebird/handler/admin/FirebirdAdminExecutorCreator.java#L38-L46), both the removed `SelectStatement` branch and the final fallback returned `Optional.empty()`. The behavior for Select, other unsupported statements, and Set/Show dispatch remains unchanged. The [existing parameterized test](https://github.com/apache/shardingsphere/blob/b3c8b5549413153907d5da4e5f732452a270c308/proxy/backend/dialect/firebird/src/test/java/org/apache/shardingsphere/proxy/backend/firebird/handler/admin/FirebirdAdminExecutorCreatorTest.java#L56-L81) directly covers Select, Set, Show, and other statements. - Firebird system-table support was implemented by [PR #36480](https://github.com/apache/shardingsphere/pull/36480). The current code registers the `system_tables` schema and uses it for Proxy metadata queries, so the original TODO is obsolete. - The MySQL unsigned-value issue was fixed by [PR #23149](https://github.com/apache/shardingsphere/pull/23149), while [PR #28849](https://github.com/apache/shardingsphere/pull/28849) added Connector/J 8 compatibility. The current version is [Connector/J 8.4.0](https://github.com/apache/shardingsphere/blob/b3c8b5549413153907d5da4e5f732452a270c308/pom.xml#L128-L130), COM_STMT_PREPARE sets the [UNSIGNED flag](https://github.com/apache/shardingsphere/blob/b3c8b5549413153907d5da4e5f732452a270c308/proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLComStmtPrepareExecutor.java#L230-L234), and the related E2E case remains enabled. - Agent E2E was migrated to Testcontainers by [PR #30898](https://github.com/apache/shardingsphere/pull/30898). The `maven-resources-plugin` is still used by the Agent E2E profiles to copy Docker build resources. This PR correctly preserves the plugin configuration and removes only the inaccurate removal notice. - The authoritative diff contains only three files, zero additions, and seven deletions. It introduces no unrelated changes and has no documentation, release, dependency, packaging, security, or performance impact. ### Review Details - Review Focus: Code Correctness Review — CI not reviewed by request. - Reviewed Scope: PR head `b3c8b5549413153907d5da4e5f732452a270c308`; base and merge-base are both `e008838c828467759c0b2a6a20c46dcb04aabac1`. The local triple-dot file list exactly matched GitHub `/pulls/39232/files`. - Not Reviewed Scope: GitHub Actions, check runs, and Actions logs. Database E2E was not run because the E2E XML change only removes a comment and does not alter any case or execution matrix. - Verification: - `./mvnw spotless:check -Pcheck -T1C` — exit 0. - `./mvnw checkstyle:check -Pcheck -T1C` — exit 0. - `./mvnw -pl proxy/backend/dialect/firebird -DskipITs -Dtest=FirebirdAdminExecutorCreatorTest test` — exit 0; 4 tests passed. - PR metadata, commits, changed files, issue comments, review comments, and reviews were inspected completely. The final adversarial pass found no additional actionable issues. - Release Note / User Docs: Not required; this is an internal, behavior-preserving cleanup of dead code and obsolete comments. -- 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]
