fix(bqjdbc): fix Long to java.sql.Time coercion#13035
fix(bqjdbc): fix Long to java.sql.Time coercion#13035
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates several JDBC methods in BigQueryBaseResultSet, BigQueryCallableStatement, and BigQueryPreparedStatement with @deprecated and @SuppressWarnings annotations to align with the JDBC specification. Additionally, it replaces a deprecated java.sql.Time constructor in BigQueryTypeCoercionUtility with a Calendar-based implementation. A review comment suggests using java.time.LocalTime and java.sql.Time.valueOf() instead of Calendar to avoid performance overhead during result set iteration, as Calendar is relatively expensive for high-frequency operations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request deprecates several JDBC methods and refactors time and timestamp coercion logic within the BigQuery JDBC driver to improve precision and compliance with the JDBC specification. Key changes include simplifying Long-to-Time and Long-to-Timestamp conversions and introducing timezone offset adjustments for wall-clock representation in Time objects. Feedback identifies a logic inconsistency between Arrow and JSON transport paths, suggests using Instant.EPOCH.plus for better readability, warns of performance overhead from repeated TimeZone.getDefault() calls in hot paths, and notes that certain unit tests have become environment-dependent.
| Timestamp.valueOf(LocalDateTime.of(2023, MARCH, 30, 11, 14, 19, 820227000)), | ||
| Date.valueOf(LocalDate.of(2023, MARCH, 30)), | ||
| Time.valueOf(LocalTime.of(11, 14, 19, 820227)), | ||
| new Time( |
There was a problem hiding this comment.
Can we use import instead of full class names? (I don't know why jetski defaults to using full names tbh, but I don't think it is a common practice)
b/510853631
chore(bqjdbc): suppress deprecation warnings