FlyingZC commented on PR #38788:
URL: https://github.com/apache/shardingsphere/pull/38788#issuecomment-4610823995
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** Reviewed PR latest head
`f54f93299b8a8506b674d41bbc3697669544e0b0`; base `master`, local merge-base
`f315a1dc7ed09d3e21e6c98c4aeeb9fa378c3462`. GitHub `/pulls/38788/
files` and local triple-dot diff matched exactly:
`features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/type/complex/ShardingCartesianRouteEngine.java`,
`features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/type/standard/ShardingStandardRouteEngine.java`.
- **Not Reviewed Scope:** Full sharding module test suite, Proxy/JDBC E2E,
performance benchmark, and GitHub Actions/CI status were not reviewed.
- **Need Expert Review:** No dedicated security, protocol, dependency, or
SQL dialect parser review required.
### Basis
- This is a behavior-preserving refactor to reduce temporary collections
and repeated object creation in sharding route engines.
- `ShardingCartesianRouteEngine` keeps the same intersection,
actual-table-group mapping, cartesian product, and route unit assembly
semantics. Reusing `RouteMapper` for the same data
source is safe because `RouteMapper` is final and immutable.
- `ShardingStandardRouteEngine` keeps the same routed data source/table
behavior. The change only passes the accumulating `Collection<DataNode>` into
`routeTables(...)` instead of
returning a temporary collection and immediately calling `addAll(...)`.
- Shared-path blast radius was checked for JDBC/Proxy sharding route usage
through standard, complex, and broadcast routing paths. The refactor does not
change routing precedence, hint
behavior, mixed condition behavior, config-disabled behavior, SQL parser
behavior, or public API/SPI contracts.
- No `ConcurrentHashMap#computeIfAbsent` usage was introduced on the
high-frequency route path.
### Verification
- `git diff --check
refs/remotes/apache/master...refs/remotes/apache/pr-38788` exited `0`.
- `./mvnw -pl features/sharding/core -am -DskipITs
-Dsurefire.failIfNoSpecifiedTests=false
-Dtest=ShardingStandardRouteEngineTest,ShardingComplexRouteEngineTest,ShardingTableBroadcastRouteEngineTest
test` exited `0`.
- 17 tests run, 0 failures, 0 errors, 1 existing disabled test skipped.
- `./mvnw -pl features/sharding/core -am -DskipITs -DskipTests
checkstyle:check -Pcheck -T1C` exited `0`.
--
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]
