terrymanu commented on PR #39034:
URL: https://github.com/apache/shardingsphere/pull/39034#issuecomment-4902786582
### Summary
**Review Result: Mergeable**
Reason: This is a code-scope only result; CI was not reviewed by request. I
did not find a code-level blocker in the latest PR head. The constructor
replacements preserve the reviewed access levels and constructor shape, and the
scoped compile plus repository style checks passed.
### Evidence
- The PR consistently replaces pure boilerplate constructors with narrow
Lombok constructor annotations, for example `@RequiredArgsConstructor` in
`database/protocol/dialect/firebird/src/main/java/org/apache/shardingsphere/database/protocol/firebird/packet/command/query/batch/FirebirdBatchStatement.java:30`,
`@RequiredArgsConstructor` in
`infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/identifier/IdentifierIndex.java:46`,
and private nested constructor replacements in
`infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/identifier/IdentifierIndex.java:329`.
- Package-private and private constructor visibility was preserved with
explicit Lombok access levels, for example
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/MCPSQLExecutionFacade.java:44`
and
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPCompletionTargetDescriptorValidator.java:39`.
- Static diff review found no deleted constructor Javadocs, constructor
annotations, validation logic, side effects, `super(...)` calls, or `this(...)`
delegation. It also found no `@AllArgsConstructor` replacement on classes with
initialized instance fields that would change the generated constructor
signature.
- The only non-constructor cleanup found was test-only stale `throws
SQLException` removal and one unused private test helper removal; the helper
had no base-branch references.
- No docs, release notes, build files, dependency metadata, distribution
files, native-image metadata, or service descriptors were changed.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: 112 modified Java files from GitHub `/pulls/39034/files`,
across `database/connector/dialect/mysql`,
`database/protocol/dialect/firebird`, `features/encrypt/core`,
`features/sharding/distsql/statement`, `infra/common`, `infra/distsql-handler`,
`infra/spi`, `mcp/bootstrap`, `mcp/core`, `mcp/features/*`, `mcp/support`,
`mode/core`, `mode/type/cluster/core`, `parser/sql/statement/core`,
`proxy/frontend/dialect/firebird`, `test/e2e/mcp`, and `test/e2e/sql`.
- Latest PR head SHA: `f12e01811383d28f042e706493a82ce6d3fd4b74`. Base SHA /
local merge-base: `32aa9fae360b1d1659a787d44b0082f49319057e`. Local triple-dot
file list matched GitHub `/pulls/39034/files` exactly: 112/112 files.
- Not Reviewed Scope: GitHub Actions, check-runs, Actions logs, and runtime
E2E execution were not reviewed because this was a Code Correctness Review.
- Verification:
- `./mvnw -pl <21 affected modules> -am -DskipTests test-compile -Pcheck
-T1C`: exit 0.
- `./mvnw spotless:check -Pcheck -T1C`: exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C`: exit 0.
- Static PR diff audits for constructor logic removal, access-level
preservation, enum replacement risk, initialized-field `@AllArgsConstructor`
risk, docs/build/distribution impact, and high-frequency `computeIfAbsent`
additions: completed with no blocker found.
- Release Note / User Docs: Not required. The PR is a code-style/boilerplate
constructor refactor and does not change user-facing behavior, configuration,
migration, diagnostics, dependency metadata, or distribution artifacts.
--
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]
