manasagar commented on PR #37965: URL: https://github.com/apache/shardingsphere/pull/37965#issuecomment-3876310152
> Thanks for the effort to make the text protocol type-aware and for adding initial time-related tests—this is the right direction. > > Change requests: > > * Root cause still unresolved: the proxy returns java.sql.Timestamp, which still goes through each.toString() and keeps the .0 suffix. Issue #37437 remains. > * Time zone semantics are broken: the OffsetDateTime branch forces withOffsetSameInstant(UTC) and emits +00, diverging from PostgreSQL’s session time zone output and risking visible offsets. > * Trailing-zero handling is inconsistent: OffsetTime always pads to 6 decimals, so 10:00:00.0+05:30 becomes 10:00:00.000000+05:30; LocalTime/LocalDateTime precision and trimming still don’t align with PostgreSQL rules. > * Type detection: relying on instanceof misses the main Timestamp path from JDBC. The formatter should pick behavior based on column type, not just runtime class. > * Tests don’t cover the failing scenario (timestamp without fractional seconds) or timestamptz/timetz behavior, so the fix isn’t demonstrated and regressions aren’t guarded. > * Please keep ResultSetUtils untouched for this issue; the correct place to fix is the PostgreSQL text protocol formatter, not the generic JDBC conversion utilities. > > Recommended next steps: > > 1. In PostgreSQLDataRowPacket.writeTextValue, format timestamp/timestamptz/time/timetz per PostgreSQL text protocol: max 6 fractional digits, trim trailing zeros, preserve session time zone. > 2. Base formatting on column type, not only runtime instanceof, so Timestamp values are handled. > 3. Add tests for the reported case and for timestamptz/timetz outputs to prove the fix. > > Please address these before we merge; happy to review the next revision. I have implemented the column value based type checking When looking into ResultSetUtil I saw that Timstamp from db can be converted into LocalDateTime or TimeStamp or String so I also added runtime formatter along with the column type to make sure correct formatting is applied. I have also changed the parser for OffsetTime and OffsetDateTime according to specifications . -- 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]
