terrymanu commented on PR #40237: URL: https://github.com/apache/shardingsphere/pull/40237#issuecomment-6311315011
### Result Review Result: Mergeable No blocking issues were found in the current code scope. The previous finding about weakened column-name assertions has been fixed. ### Evidence - The current implementation normalizes only expected column names according to database rules. Actual JDBC column names are still compared exactly, preserving detection of incorrect casing. [DDLE2EIT.java:408](https://github.com/apache/shardingsphere/blob/c30541c0adf297bdc6b178d2accc489b2451d727/test/e2e/sql/src/test/java/org/apache/shardingsphere/test/e2e/sql/it/sql/ddl/DDLE2EIT.java#L408) - Firebird is already supported by the project. This PR adds SQL E2E coverage for `Docker / proxy / Cluster / db`. [Existing support documentation](https://github.com/apache/shardingsphere/blob/c30541c0adf297bdc6b178d2accc489b2451d727/README.md#L279) - Complete paths for container initialization, connection validation, cleanup commits, deterministic ordering, SPI registration, and dependency packaging were verified. Other databases retain their existing default behavior. ### Coverage - **Candidate**: Public PR with no local code delta. Head: `c30541c0adf297bdc6b178d2accc489b2451d727`. - **Baseline**: Base `5983662c26672736f633e0e999238617407d1fef`; merge-base `01446d4fad48450e7fc3eb3b3631b25d6bf411df`. - **Scope and requirements**: GitHub’s file inventory matches the local triple-dot diff. All **28 files and 41 changed blocks** were reviewed against the PR description, maintained Firebird support documentation, `CODE_OF_CONDUCT.md`, and applicable repository rules. - **Review completeness**: Completed root-cause and behavior, blast-radius, and tests/runtime discovery lenses, followed by convergence. Coverage includes initialization, connections, cleanup and ordering, DDL assertions, scenario data, and execution configuration. No unresolved outcome-sensitive evidence gaps remain. - **Independent confirmation**: Completed three rounds comprising six independent assessments, with individual coordinator review. The Native initialization concern was verified against the baseline call path as an already unsupported path. The final two rounds agreed on the complete assessment; `validate-session` exited 0. Local style checks are summarized below. Commands omit only exact file-filter arguments; actual XML reports and indexes prove that every intended file was selected. | Check | Command summary | Result | |---|---|---| | Checkstyle | `./mvnw -B -ntp -nsu -pl .,test/e2e/fixture,test/e2e/env,test/e2e/sql … spotless:check checkstyle:check -Pcheck -T1C` | Exit 0; 9 Java files, no errors | | Spotless | `./mvnw -B -ntp -nsu -pl .,test/e2e/fixture,test/e2e/env,test/e2e/sql … spotless:check -Pcheck -T1C` | Exit 0; all 12 files passed | Both checks covered the following Java files. Relative to `test/e2e/env/src/test/java/org/apache/shardingsphere/test/e2e/env/`: - `container/storage/option/StorageContainerConnectOption.java` - `container/storage/option/dialect/firebird/FirebirdStorageContainerConnectOption.java` - `container/storage/option/dialect/firebird/FirebirdStorageContainerCreateOption.java` - `container/storage/option/dialect/firebird/FirebirdStorageContainerOption.java` - `container/util/StorageContainerUtils.java` Relative to `test/e2e/sql/src/test/java/org/apache/shardingsphere/test/e2e/sql/`: - `env/DataSetEnvironmentManager.java` - `framework/metadata/DialectQueryBehaviorProvider.java` - `framework/metadata/dialect/FirebirdDialectQueryBehaviorProvider.java` - `it/sql/ddl/DDLE2EIT.java` Spotless additionally covered `test/e2e/fixture/pom.xml`, `test/e2e/sql/pom.xml`, and `.github/workflows/nightly-e2e-sql.yml`. These three files are not applicable to Checkstyle. The remaining 16 files—release notes, 2 SPI descriptors, 4 SQL files, 8 case XML files, and 1 scenario YAML file—fall outside both tools’ configured scope and are classified as **Not Applicable**. This conclusion covers code correctness only: **CI was not reviewed, and Docker E2E was not rerun**. Local style verification was completed. No PR code was modified or remote feedback posted. Temporary review materials were cleaned up. -- 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]
