RaigorJiang commented on PR #40232: URL: https://github.com/apache/shardingsphere/pull/40232#issuecomment-6258690518
### Result Review Result: Mergeable The complete review of [PR #40232](https://github.com/apache/shardingsphere/pull/40232) found no blocking issues or outstanding recommended changes. The fix addresses the owning parameter-decoding boundary: it uses known target-column metadata to decode `MYSQL_TYPE_STRING` parameters as strings and leaves numeric conversion to MySQL. The grouped switch preserves the existing character-column behavior and covers the intended numeric types without changing public APIs, SPIs, or dependencies. This is a **Code Correctness Review**. The result indicates code-scope readiness; remote CI was neither queried nor assessed. ### Evidence - **The fix addresses the root cause.** The complete path was inspected: target-column types recorded during `COM_STMT_PREPARE` → parameter decoding during `COM_STMT_EXECUTE` → JDBC `setObject()` binding. String parameters targeting numeric columns now retain string semantics, addressing the binary-binding behavior reported in [#40227](https://github.com/apache/shardingsphere/issues/40227). The implementation performs no Java numeric parsing, so numeric range handling and invalid numeric-string conversion remain MySQL’s responsibility. - **Existing behavior is protected.** - The `STRING`, `VAR_STRING`, and `VARCHAR` target-column cases continue to decode as strings, preserving the behavior introduced by [#38009](https://github.com/apache/shardingsphere/pull/38009) for [#37993](https://github.com/apache/shardingsphere/issues/37993). - Binary-column parameters retain their original bytes. JDBC binary types map to BLOB enum values excluded from string decoding. - Missing or unknown column metadata, NULL parameters, stored parameter-type reuse, and long-data handling retain their existing behavior. - The type predicate introduces no loop, collection allocation, synchronization, or I/O. The additional string construction applies only to the numeric-column path whose semantics require correction. - **Unit tests were rerun for the reviewed head.** Using Maven **3.10.0**, as specified by the PR’s wrapper configuration, all **487 tests** in the MySQL protocol module passed, including **49 tests** in `MySQLComStmtExecutePacketTest`. There were no failures, errors, or skipped tests. Coverage for `MySQLComStmtExecutePacket` was **54/54 lines** and **57/57 branches**. - **Local style checks were rerun.** Spotless and Checkstyle both exited with **0**. Their actual selected scope was verified to include both Java files changed by the PR. - **The existing real-environment evidence remains applicable.** The public PR source content and the currently compiled packet and switch classes match the artifact previously tested against MySQL. Those results were reused after this identity check; Proxy was not restarted during this review: - All 12 original Go/GORM query cases passed for each backend configuration: `useServerPrepStmts=false` and `true`. - Both BIGINT/VARCHAR composite sharding-key fixtures passed under each configuration, exercising the sharding-value `Comparable` check relevant to #37993. - The #37993 validation covers an equivalent failure path on an existing table, rather than reconstructing the original issue’s complete schema and topology. - All fixtures were rolled back, and both temporary Proxy instances were stopped. ### Coverage - **Reviewed public head:** `175cb189f465e7fe9518048b179473b354104792`. The head, base, and PR description were checked again at the end of the review and had not changed. - **Comparison basis:** the public base was `319a369d4a5488e30ba74bc04fccaceea4aedf12`; GitHub’s compare API established the merge-base as `0cbdde72383415d3b33be763daddf362dc89b944`. - **Complete authoritative file scope:** GitHub’s changed-file list matched the local triple-dot comparison, and both local file blobs matched the public PR: 1. `database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java` 2. `database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java` - **Excluded local changes:** uncommitted Markdown development references were excluded. The unrelated local Maven wrapper version change was also excluded; verification directly invoked Maven 3.10.0 to avoid its influence. - **Verification commands:** - Unit tests: `mvn -B -pl database/protocol/dialect/mysql -DskipITs -Dspotless.skip=true -Djacoco.skip=false -Djacoco.append=false -DargLine=-Xmx1g test jacoco:report` — exit **0**, with a separate coverage-data file for this review. - Style checks: `mvn -B -X -pl database/protocol/dialect/mysql -Pcheck -T1C -DspotlessFiles=<exact absolute-path regexes for both listed files> -Dcheckstyle.includes=<both source-root-relative Java paths> -Dcheckstyle.includeResources=false -Dcheckstyle.includeTestResources=false spotless:check checkstyle:check` — exit **0**. Path filters are sanitized here; the actual execution selected one production file and one test file. - **Review completeness:** root cause and behavior, blast radius and contracts, and tests and runtime were reviewed across the complete candidate. The final convergence pass found no additional issues, and no outcome-sensitive evidence gaps remain. The review did not modify source files, the Git index or refs, or remote state. -- 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]
