strawberry1314 commented on PR #34332: URL: https://github.com/apache/shardingsphere/pull/34332#issuecomment-3594322193
> Thanks for the fix — the direction of addressing the byte[] pagination parameter issue is absolutely correct. The original problem (Go driver / binary protocol producing a byte[] for LIMIT/OFFSET and causing a ClassCastException → NPE in MySQLComStmtExecuteExecutor) is real, and adding an extra branch for byte[] makes sense. > > One small concern: The current implementation extracts the numeric value by using only the first byte: > > ((byte[]) obj)[0] - '0' > > This works for single-digit limits, but may lead to incorrect pagination values for multi-digit inputs like 10, 100, etc. For such cases the method would return 1 instead of the full number. This silently changes query results, which could be more dangerous than the original explicit failure. > > To make the behavior more robust, it would be safer to parse the full value, e.g.: > > Treat the byte[] as a UTF-8 numeric string (new String(bytes) + Long.parseLong), or > > Decode according to the upstream protocol rules if the bytes represent a binary-encoded integer. > > This keeps the fix correct while avoiding potential functional regressions. > > Everything else in the PR looks good — thanks again for working on this! If needed, I’m happy to help with additional test coverage (e.g., LIMIT ≥ 10). > Thanks for the fix — the direction of addressing the byte[] pagination parameter issue is absolutely correct. The original problem (Go driver / binary protocol producing a byte[] for LIMIT/OFFSET and causing a ClassCastException → NPE in MySQLComStmtExecuteExecutor) is real, and adding an extra branch for byte[] makes sense. > > One small concern: The current implementation extracts the numeric value by using only the first byte: > > ((byte[]) obj)[0] - '0' > > This works for single-digit limits, but may lead to incorrect pagination values for multi-digit inputs like 10, 100, etc. For such cases the method would return 1 instead of the full number. This silently changes query results, which could be more dangerous than the original explicit failure. > > To make the behavior more robust, it would be safer to parse the full value, e.g.: > > Treat the byte[] as a UTF-8 numeric string (new String(bytes) + Long.parseLong), or > > Decode according to the upstream protocol rules if the bytes represent a binary-encoded integer. > > This keeps the fix correct while avoiding potential functional regressions. > > Everything else in the PR looks good — thanks again for working on this! If needed, I’m happy to help with additional test coverage (e.g., LIMIT ≥ 10). Thanks for the thorough review and for catching this important issue with multi-digit values. You're absolutely right, I have made the modifications. Besides, thank you for helping to add additional test coverage . -- 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]
