RaigorJiang commented on PR #40322: URL: https://github.com/apache/shardingsphere/pull/40322#issuecomment-6680349235
### Summary The PR preserves fractional seconds in ordinary within-day MySQL TIME results by reading `LocalTime` before merging and serialization. It uses the existing dialect mapper boundary, keeps legacy JDBC/Calcite representations at their consumer boundaries, and adapts the affected encoders and runtime metadata. The current ownership and reader lifecycle are appropriate for the scoped fix. ### Result Review Result: Mergeable No blocking correctness, architecture, performance, or applicable coding-standard issue remains in the reviewed public head. The previously reported returned-local naming issue is fixed. ### Evidence - **Latest correction:** [`MySQLTimeValueReader.java:69–74`](https://github.com/apache/shardingsphere/blob/faa47a2da55de8829fa31e353d97270b6e940b0f/database/connector/dialect/mysql/src/main/java/org/apache/shardingsphere/database/connector/mysql/resultset/MySQLTimeValueReader.java#L69-L74) consistently uses `result`, preserving SQL NULL handling, epoch conversion, and the non-epoch fallback. - **Compatibility:** Dialect opt-in preserves native non-MySQL reads. The SQL TIME metadata gate protects generic `LocalTime` conversions, while JDBC/Calcite adapters and legacy serializer branches retain their established contracts. - **Architecture and performance:** Capability state is scoped to each result set and column; SPI lookup is outside per-value reading. No additional result scan, SQL/network call, lock, or unbounded global cache was identified in the changed paths. - **Style verification:** Valid Checkstyle and Spotless evidence covers all **41 changed Java files**, with **0 Checkstyle errors and 14 advisory warnings**. - **Unit verification:** Source-matched owner-suite evidence covers **5,578 tests, 0 failures, 0 errors, and 1 existing skip**, including the latest **100-test MySQL connector rerun** without double-counting it. Existing results were reused after verifying that source, effective configuration, tool inputs, and scope match the pushed candidate; no Maven checks were rerun solely for the push. ### Coverage Reviewed public PR head `faa47a2da55de8829fa31e353d97270b6e940b0f` against base `a6dfa0c1f63a79ac2b4c81289139ff15c8d902fa`: **41 authoritative files and 220 changed blocks**, with complete independent review and validated consolidation. Local HEAD matched the public head; uncommitted Markdown was excluded, and no source or Git state was modified. This is a **Code Correctness Review**; CI was not reviewed, and no fresh Proxy/database integration run was performed. Negative or at-least-24-hour TIME durations, fixed-width fractional text, PREPARE/declared precision metadata, and broader DATETIME/TIMESTAMP work remain outside this PR’s stated scope. -- 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]
