terrymanu commented on PR #38908:
URL: https://github.com/apache/shardingsphere/pull/38908#issuecomment-4792374599
### Summary
**Review Result: Mergeable**
Reason: The latest head fixes the previously observed `WhereExtractor`
regression, keeps the local-variable `final` and Javadoc cleanup aligned with
the project style rules, and passes the relevant local style, sentinel, and
full CI-equivalent verification on a clean latest-PR checkout. I did not find
remaining merge-blocking scope, behavior, or documentation issues.
### Evidence
- GitHub `/pulls/38908/files` reports 89 changed files, and the local
triple-dot diff from merge-base `0feebf9a4da80a70053a62f444b989c4753c233a` to
latest head `0f759721b281a959a777e07b917e48760517edb2` matches that file list.
- The previous runtime failure path in
`parser/sql/statement/core/src/main/java/org/apache/shardingsphere/sql/parser/statement/core/extractor/WhereExtractor.java`
is fixed: the latest code avoids the overloaded method reference and uses the
explicit `isPresent()` branch. The targeted `ShardingDDLResultMergerTest` now
passes with `-am`, so the dependent parser/binder bytecode is fresh.
- AST scanning of the 89 changed Java files found no local variable `final`,
no lambda parameter `final`, and no override-method Javadocs. The remaining
constructor Javadocs are for non-obvious constructor semantics, such as
temporary identifier-context behavior and specialized workflow/classification
constructors.
- High-risk declaration-move areas were reviewed, including MySQL handshake
payload parsing, schema/table identifier refresh, migration commit/rollback
timing, proxy context restoration, MCP test captors, and E2E resource lifecycle
refactors. I did not find a behavior-changing regression after the latest fix.
- Latest GitHub check-runs had no failures at review time; some remote
E2E/GraalVM checks were still in progress. Local full CI-equivalent
verification passed on the latest PR head.
### Review Details
- Reviewed Scope: PR #38908 latest head
`0f759721b281a959a777e07b917e48760517edb2`; base `master` at
`0feebf9a4da80a70053a62f444b989c4753c233a`; merge-base
`0feebf9a4da80a70053a62f444b989c4753c233a`. Reviewed all 89 changed files from
the GitHub file list and matching local triple-dot diff, with focused review on
style-rule compliance and behavior-sensitive declaration moves.
- Not Reviewed Scope: Final completion of the still-running remote Docker
E2E, Agent E2E, MCP runtime, and GraalVM checks. No failure was present on the
latest head when reviewed, and the local full CI-equivalent command passed.
- Verification:
- AST scan over changed Java files: exit 0; local `final` declarations 0,
lambda `final` parameters 0, override Javadocs 0.
- `./mvnw spotless:check -Pcheck -T1C`: exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C`: exit 0.
- `./mvnw -pl features/sharding/core -am
-Dtest=ShardingDDLResultMergerTest -DskipITs -Dspotless.skip=true
-Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false test -Pcheck
-T1C`: exit 0.
- `./mvnw clean install -T1C -B -ntp -fae`: exit 0.
- Release Note / User Docs: Not required. This PR is an internal code-style
cleanup and test/refactor cleanup without user-facing behavior, configuration,
API/SPI, SQL syntax, dependency, or distribution changes.
--
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]
