RaigorJiang commented on PR #39204: URL: https://github.com/apache/shardingsphere/pull/39204#issuecomment-5032677017
### Summary **Review Result: Mergeable** Reason: The change correctly restores MySQL `COM_STMT_EXECUTE` signedness ownership to the client-supplied parameter type flag, rather than deriving it from the target column metadata returned during `COM_STMT_PREPARE`. The implementation remains narrowly scoped, preserves the prior BLOB byte-preservation path, removes only now-invalid cached target-column flag state, and includes focused regression coverage. This result is based on code scope and local verification only; CI not reviewed by request. ### Evidence - The [MySQL `COM_STMT_EXECUTE` protocol](https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_stmt_execute.html) defines a type-and-flag pair for each newly bound parameter, with the flag byte's high bit representing unsignedness. - `MySQLComStmtExecutePacket` now decodes numeric values from `MySQLPreparedStatementParameterType#getUnsignedFlag()` at `database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java:127`. This prevents an `INT` parameter carrying `0xFFFFFFFF` with client flag `0x00` from being decoded as unsigned merely because the target column is `INT UNSIGNED`. - `MySQLComStmtExecuteExecutor` stores the full client parameter type object when type information is supplied and reuses that object for subsequent executions at `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/execute/MySQLComStmtExecuteExecutor.java:108`. Both the new-type and cached-type paths therefore retain the client unsigned bit. - `MySQLComStmtPrepareExecutor` still calculates and emits target column flags in the `COM_STMT_PREPARE` response, while no longer retaining those flags as execution-decoding state at `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLComStmtPrepareExecutor.java:141`. - The packet-level regression tests use the same `MYSQL_TYPE_LONG` payload bytes for signed and unsigned cases, differing only in the parameter flag: `0x00` decodes to `-1`; `0x80` decodes to `4294967295L` at `database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:72`. - The existing BLOB regression matrix remains covered for `STRING`, `VAR_STRING`, and `VARCHAR` against `TINYBLOB`, `BLOB`, `MEDIUMBLOB`, and `LONGBLOB` target columns at `database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:110`. - Public PR discussion and review endpoints contained no prior review feedback to reconcile. ### Review Details - Review Focus: Code Correctness Review; CI not reviewed by request. - Reviewed Scope: PR head `7a74fc1bec2fc96d57d89304e0769367cd5fbd07`, merge base `5702a14ade73a3b5eeb34872af672ef5a9e1f264`; the GitHub `/files` inventory matched the local triple-dot diff: - `RELEASE-NOTES.md` - `database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java` - `database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java` - `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/MySQLServerPreparedStatement.java` - `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/execute/MySQLComStmtExecuteExecutor.java` - `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLComStmtPrepareExecutor.java` - `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/MySQLCommandPacketFactoryTest.java` - `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/admin/MySQLComResetConnectionExecutorTest.java` - `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/MySQLComStmtSendLongDataExecutorTest.java` - `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/execute/MySQLComStmtExecuteExecutorTest.java` - `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLPreparedStatementMetadataFactoryTest.java` - `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/reset/MySQLComStmtResetExecutorTest.java` - Not Reviewed Scope: CI/workflow status, full distribution packaging, and external client-to-server integration environments were not reviewed. - Verification: - `./mvnw -pl database/protocol/dialect/mysql,proxy/frontend/dialect/mysql -DskipITs -DskipIT -DskipE2E -Dtest=MySQLComStmtExecutePacketTest,MySQLComStmtExecuteExecutorTest,MySQLCommandPacketFactoryTest,MySQLComResetConnectionExecutorTest,MySQLComStmtSendLongDataExecutorTest,MySQLPreparedStatementMetadataFactoryTest,MySQLComStmtResetExecutorTest,MySQLComStmtPrepareExecutorTest -DfailIfNoTests=true -Dsurefire.failIfNoSpecifiedTests=false test -Pcheck -T1C` — exit code `0`; protocol packet tests: 26 passed, Proxy targeted tests: 55 passed. - `./mvnw spotless:apply -Pcheck -T1C` — exit code `0`. - `./mvnw checkstyle:check -Pcheck -T1C` — exit code `0`. - Release Note / User Docs: `RELEASE-NOTES.md` includes an accurate Proxy bug-fix entry linked to `#39204`; no separate user documentation change is needed for this internal protocol-decoding correction. -- 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]
