RaigorJiang commented on PR #40236: URL: https://github.com/apache/shardingsphere/pull/40236#issuecomment-6267499583
### Result Review Result: Mergeable The latest public head of PR [#40236](https://github.com/apache/shardingsphere/pull/40236), `e3703f2a8e30b433d6de6484b5cfac2e57b2ad3b`, has undergone a complete independent review. No blocking issues or unresolved evidence gaps affecting the conclusion were found. This conclusion establishes code-scope readiness. The review used the Code Correctness Review focus; GitHub CI status was outside its scope. ### Evidence 1. **The fix addresses the actual precision-loss path.** Previously, the encoder treated a `Timestamp` as a `Date` and reconstructed it through `getTime()`, discarding submillisecond precision before serialization. The current implementation preserves the original `Timestamp` and calls `toLocalDateTime()`, allowing its complete fractional component to reach the existing microsecond encoder. The repair remains within the existing MySQL temporal encoder and introduces no public API, configuration, or extension contract. [Conversion implementation](https://github.com/apache/shardingsphere/blob/e3703f2a8e30b433d6de6484b5cfac2e57b2ad3b/database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/protocol/MySQLDateBinaryProtocolValue.java#L67-L76) 2. **Compatibility paths and applicable written standards are preserved.** The implementation explicitly retains the `LocalDate` and `LocalDateTime` checks and the generic `Date` fallback. The existing 4-, 7-, and 11-byte content layouts remain unchanged; midnight values containing microseconds still use the 11-byte layout. NULL handling remains upstream, and TIME uses a separate encoder. The final `else` contains two meaningful conversion steps, so the rule requiring a ternary expression for branches that only assign the same variable does not apply to the entire chain. `@HighFrequencyInvocation` is consistent with the per-row, per-temporal-column execution path and has SOURCE retention. Inspection found no credible performance cost growth. 3. **The tests directly protect the wire format.** They use a real `MySQLPacketPayload` and `ByteBuf` to verify the length, date/time fields, little-endian microseconds, and absence of trailing bytes. The buffer is released in `finally`. Existing scenarios remain covered, with additional regression cases for six-digit fractions, a single microsecond, fractional trailing zeros, and midnight with microseconds. [Byte assertions](https://github.com/apache/shardingsphere/blob/e3703f2a8e30b433d6de6484b5cfac2e57b2ad3b/database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/protocol/MySQLDateBinaryProtocolValueTest.java#L83-L105), [test vectors](https://github.com/apache/shardingsphere/blob/e3703f2a8e30b433d6de6484b5cfac2e57b2ad3b/database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/protocol/MySQLDateBinaryProtocolValueTest.java#L161-L1 77) 4. **The original verification evidence remains valid for the current public commit.** This review did not rerun tests, style checks, packaging, or Proxy services. Before reusing actual executions from the same session, the reviewers independently revalidated the public blobs, current source files, effective configuration, tool identities, raw logs, and encoder class bytes in the archive, extracted distribution, and current compiled output. | Verification | Confirmed result | |---|---| | Target-class unit tests | 22 passed; 0 failures, errors, or skips | | MySQL protocol module tests | 494 passed; 0 failures, errors, or skips | | Target-class JaCoCo coverage | 65/65 lines and 29/29 branches | | Checkstyle | Selected 1 production and 1 test file; processed 2 files; 0 violations; exit code 0 | | Spotless | Explicitly selected both changed files; both cache-clean; unchanged inputs verified; exit code 0 | | Proxy distribution | Original build exited 0; archive and encoder class identities match | | Real-environment verification | 112 query comparisons; 0 precision mismatches; 224 NULL/DATE checks | The real-environment evidence uses MySQL 8.0.37 and Connector/J 8.4.0. It covers all four backend combinations of `useServerPrepStmts` and `treatMysqlDatetimeAsTimestamp`, direct MySQL and Proxy connections, and text and actual `ServerPreparedStatement` paths. The measured scope is UTC `DATETIME(6)` constants plus DATE/NULL checks. Physical TIMESTAMP columns, other time zones, TIME, and metadata are outside this runtime coverage. The PR description’s `64/64` line-coverage count is outdated. The current raw report records **65/65 lines** and **29/29 branches**. This is a nonblocking documentation discrepancy; full coverage and code readiness remain supported. ### Coverage - **Candidate and baseline:** The review covers the complete public PR, including both commits. The reviewed head is `e3703f2a8e30b433d6de6484b5cfac2e57b2ad3b`, the public base is `5983662c26672736f633e0e999238617407d1fef`, and the effective diff baseline is merge-base `5f28295f952d446f86694ed3967e41cec3b870ed`. The head, description, and changed-file scope were reconfirmed through the GitHub API before publication. - **Complete file scope:** Both authoritative GitHub files, all 14 changed blocks, and the affected producer-to-consumer path were reviewed. The local reviewed files match the public blobs, with no additional local delta: - `database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/protocol/MySQLDateBinaryProtocolValue.java` - `database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/protocol/MySQLDateBinaryProtocolValueTest.java` - **Requirements and contracts:** The assessment uses the public problem description, the existing JDBC-object-to-binary-row execution path, and the maintained factory, encoder, and test contracts. The change restores existing microsecond output support without introducing a new project commitment. - **Discovery and risk coverage:** All three complete discovery lenses—root cause and behavior, blast radius and contracts, and tests and runtime—were completed. Functional boundaries, compatibility, test validity, concurrency and performance, security and operations, dependencies, and packaging were checked. Pre-existing, unchanged concerns such as synthetic overflow fixtures and helper ordering were distinguished from PR-introduced issues. - **Independent confirmation:** Three independent review pairs completed full assessments, followed by substantive review of their reports and counter-evidence. The final two adjudications agree on every historical candidate’s disposition. Final convergence found no new independent issues. Session validation exited 0, and review-owned temporary materials were cleaned up. - **Style verification scope:** Both changed files are governed by both Checkstyle and Spotless; neither check is Not Applicable. The verified original passing command is represented below with repository-root variables for the same exact file selection: ```sh review_module=database/protocol/dialect/mysql review_package=org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/protocol review_class=MySQLDateBinaryProtocolValue review_main="$PWD/$review_module/src/main/java/$review_package/$review_class" review_test="$PWD/$review_module/src/test/java/$review_package/${review_class}Test" ./mvnw -B -X -o -pl "$review_module" -Pcheck -T1C \ "-DspotlessFiles=$review_main\.java,$review_test\.java" \ "-Dcheckstyle.includes=$review_package/$review_class.java,$review_package/${review_class}Test.java" \ -Dcheckstyle.includeResources=false \ -Dcheckstyle.includeTestResources=false \ spotless:check checkstyle:check ``` Both checks passed, and the combined command exited 0. Local style verification was assessed independently of GitHub CI, which was not reviewed. No repository or remote state was modified; existing local reference documents and unrelated changes were preserved. -- 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]
