strongduanmu commented on PR #38925:
URL: https://github.com/apache/shardingsphere/pull/38925#issuecomment-4807645577
### Summary
**Review Result: Mergeable**
Reason: This PR has a very small change scope, and the root-cause path is
clear. `InsertValueContext` now resolves the local parameter index by first
matching the parameter marker order within the current value context, avoiding
incorrect value lookup when the global marker index differs from the local
parameter-list index. The tests and local verification cover the core path, and
I did not find any merge-blocking issue.
### Evidence
-
`infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/insert/values/InsertValueContext.java:103`
now first locates the marker by its order in the current
`parameterMarkerExpressions`, then falls back
to `parameterMarkerIndex - parametersOffset`. This fixes the risk of
treating a global marker index as a local parameter index.
-
`infra/binder/core/src/test/java/org/apache/shardingsphere/infra/binder/context/segment/insert/values/InsertValueContextTest.java:63`
adds a scenario with global marker indexes `1, 2, 3`, verifying that column
index `1` resolves to
the local parameter value `"business_code"`.
- I checked shared consumer paths, including
`features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/rewrite/parameter/rewriter/EncryptInsertValueParameterRewriter.java:82`
and `features/encrypt/core/src/main/java/org/
apache/shardingsphere/encrypt/rewrite/token/generator/insert/EncryptInsertValuesTokenGenerator.java:159`.
This PR does not introduce target-feature leakage, new public types,
configuration changes, dependency changes, or release
artifact changes.
### Review Details
Reviewed Scope: PR head `a1b3bc6e862c42840b049d3fbbeeddb4f6199faf`,
merge-base `0023d933d18fddbfdfd9c8e17eb60437817cb253`; reviewed
`infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/insert/values/
InsertValueContext.java` and
`infra/binder/core/src/test/java/org/apache/shardingsphere/infra/binder/context/segment/insert/values/InsertValueContextTest.java`.
The local triple-dot file list matched GitHub `/pulls/38925/files`.
Not Reviewed Scope: I did not rerun the full E2E matrix locally. Broader
encrypt/sharding/shadow runtime integration was reviewed statically through
`InsertValueContext` consumers.
Verification:
- `./mvnw -pl infra/binder/core -Dtest=InsertValueContextTest -DskipITs
-Dmaven.javadoc.skip=true -Djacoco.skip=true -Dspotless.check.skip=true
-Dcheckstyle.skip=true test` exit `0`; 6 tests passed.
- `./mvnw -pl infra/binder/core -Dtest=InsertStatementContextTest
-DskipITs -Dmaven.javadoc.skip=true -Djacoco.skip=true
-Dspotless.check.skip=true -Dcheckstyle.skip=true test` exit `0`; 13 tests
passed.
- `./mvnw -pl infra/binder/core -Pcheck -DskipTests
-Dmaven.javadoc.skip=true -Djacoco.skip=true spotless:check checkstyle:check`
exit `0`; Spotless and Checkstyle passed.
- `gh pr checks 38925 --repo apache/shardingsphere` exit `8` because some
E2E jobs were still pending when queried. No failing checks were observed in
the queried output, and the relevant CheckStyle, Spotless, and CI checks had
passed.
Release Note / User Docs: Not required. The PR only changes internal
binder parameter-index resolution and does not introduce user-visible
configuration, syntax, migration, dependency, or distribution behavior.
--
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]
