terrymanu commented on PR #40072: URL: https://github.com/apache/shardingsphere/pull/40072#issuecomment-6209186797
### Result Review Result: Mergeable No blocking issues were found in the current version. The previous regression that incorrectly replaced part of the table prefix has been fixed. This conclusion covers code correctness only; CI was not reviewed. ### Evidence - [SQLHintUtils.java:137](https://github.com/apache/shardingsphere/blob/ab730c0cbda2fb10a1b03de29577dd67c3b5e53a/infra/common/src/main/java/org/apache/shardingsphere/infra/hint/SQLHintUtils.java#L137) normalizes only the property name after the final dot, preserving the table prefix and mapping aliases to the canonical keys expected by HintValueContext. - This restores the [documented alias compatibility](https://github.com/apache/shardingsphere/blob/ab730c0cbda2fb10a1b03de29577dd67c3b5e53a/docs/document/content/user-manual/common-config/sql-hint.en.md#L25), without introducing a public API or new project commitment. - All 47 tests in SQLHintUtilsTest and HintValueContextTest passed. Running the same tests against the baseline produced exactly four failures covering global and table-specific aliases, confirming that the tests detect the original defect. Both scenarios involving table names containing alias text passed on the current version. - Test command: `./mvnw -o -pl infra/common -DskipITs -Dspotless.skip=true -Dtest=SQLHintUtilsTest,HintValueContextTest -Dsurefire.failIfNoSpecifiedTests=false test`, exit code 0. ### Coverage - Candidate: an exact source snapshot of the public PR, with no additional local changes. - Head: `ab730c0cbda2fb10a1b03de29577dd67c3b5e53a` - Base / merge-base: `5756b72bfa9da47c24c4856915fd8159ce60a1be` - All three files in the complete GitHub changed-file list were reviewed. Snapshot contents match their API blob identities: - `RELEASE-NOTES.md:115` - `infra/common/src/main/java/org/apache/shardingsphere/infra/hint/SQLHintUtils.java:88` - `infra/common/src/test/java/org/apache/shardingsphere/infra/hint/SQLHintUtilsTest.java:61` - Review requirements came from CODE_OF_CONDUCT, applicable implementation, testing and non-regression rules, the SQL Hint documentation, and #40068. Coverage included alias extraction, context lookup, JDBC/Proxy propagation, routing consumers, regression tests, and release notes. Root-cause, contract/blast-radius, and test/runtime discovery, independent review, and final convergence are complete. No unresolved evidence gaps remain. - Local style verification covered both changed Java files in `infra/common`. SF represents their exact absolute-path regular expressions; CS represents their paths relative to the Java source roots; CS0 represents the initial filter with an incorrect additional `java/` prefix: - Spotless: `./mvnw -B -pl infra/common -DspotlessFiles="$SF" -Dcheckstyle.includes="$CS0" -Dcheckstyle.includeResources=false -Dcheckstyle.includeTestResources=false spotless:check checkstyle:check -Pcheck -T1C`, exit code 0. Output confirmed that both files were clean. Checkstyle selected no files in this invocation, so that Checkstyle result was not accepted as evidence. - Corrected Checkstyle verification: `./mvnw -B -pl infra/common -Dcheckstyle.includes="$CS" -Dcheckstyle.includeResources=false -Dcheckstyle.includeTestResources=false checkstyle:check -Pcheck -T1C`, exit code 0. The report confirmed that both files were checked, with zero errors. - `RELEASE-NOTES.md`: both checks are Not Applicable because the current configuration does not govern Markdown. - The final freshness check confirmed that the head, base, and changed-file list were unchanged. Temporary materials were cleaned up. No repository or remote state was modified. -- 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]
