menghaoranss commented on PR #38673:
URL: https://github.com/apache/shardingsphere/pull/38673#issuecomment-4386984448
## Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope**
- Latest PR head: `48d244a3b9f4cb344160ed1b49bae07d93bc1cf4`
- Files reviewed:
-
`features/mask/core/src/main/java/org/apache/shardingsphere/mask/rule/MaskRule.java`
-
`features/mask/core/src/test/java/org/apache/shardingsphere/mask/rule/MaskRuleTest.java`
- PR evidence:
- [PR](https://github.com/apache/shardingsphere/pull/38673)
-
[files](https://api.github.com/repos/apache/shardingsphere/pulls/38673/files)
-
[commits](https://api.github.com/repos/apache/shardingsphere/pulls/38673/commits)
-
[reviews](https://api.github.com/repos/apache/shardingsphere/pulls/38673/reviews)
-
[comments](https://api.github.com/repos/apache/shardingsphere/pulls/38673/comments)
- **Not Reviewed Scope**
- No JDBC/Proxy integration-level validation in this round (scope is
unit-level mask rule logic).
- **Need Expert Review**
- No
## Basis
- The root-cause fix is in place:
- `partialUpdate(...)` now calls `handleUpdatedMaskAlgorithm(...)`,
covering the missing path for updating existing algorithm keys.
- The test now proves real behavior change:
- `assertPartialUpdateWithToBeUpdatedAlgorithms` validates masking output
before and after update (`12**56` -> `1####6`), confirming update effectiveness.
- Risk scan:
- Design risk: low (localized change).
- Performance risk: low (local `computeIfPresent`, config-scale linear
work).
- Compatibility risk: low (no API/SPI or external config contract change).
- Regression risk: covered by add/remove/update table/algorithm test
scenarios.
## Explicit Adversarial Pass
- **Cross-dialect / adjacent-feature regression**: not observed (change is
inside `features/mask/core`, not shared parser/binder/common paths).
- **Config-disabled path**: unchanged; if mask rule is disabled, this logic
is not executed.
- **Original symptom partial coverage**: resolved by direct
behavior-difference assertion, not just existence checks.
## Verification
1. `./mvnw -pl features/mask/core -am -DskipITs -Dspotless.skip=true
-Dtest=org.apache.shardingsphere.mask.rule.MaskRuleTest
-Dsurefire.failIfNoSpecifiedTests=false test`
- Exit code: `0`
2. `./mvnw -pl features/mask/core -Pcheck -DskipTests checkstyle:check`
- Exit code: `0`
## Multi-round Comparison
- **Closed**
- “Test did not prove algorithm update takes effect” is resolved.
- “Missing dependent-module build evidence (`-am`)” is resolved.
- **Unresolved**
- None blocking in current PR scope.
- **Newly Introduced**
- None observed.
--
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]
