feat: BigQueryTemporalUtility and temporal wiring (Phase 2) - #13952
feat: BigQueryTemporalUtility and temporal wiring (Phase 2)#13952Neenu1995 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces BigQueryTemporalUtility to handle conversions between BigQuery temporal strings and JDBC types, and registers these temporal types (DATE, DATETIME, TIMESTAMP, and TIME) in BigQueryTypeRegistry. The review feedback highlights three main improvement opportunities: first, ensuring the fallback parsing in boxTimestamp handles the ' UTC' suffix robustly to avoid an IllegalArgumentException; second, using legacy Calendar manipulation for Time conversions when no timezone is provided to properly handle Daylight Saving Time; and third, implementing a manual timezone-aware conversion when mapping Time to LocalTime to prevent millisecond truncation.
| try { | ||
| return Timestamp.from(Instant.parse(iso)); | ||
| } catch (java.time.format.DateTimeParseException e) { | ||
| // Fallback for non-standard formats | ||
| return Timestamp.valueOf(val); | ||
| } |
There was a problem hiding this comment.
If Instant.parse(iso) fails and throws a DateTimeParseException, the fallback logic calls Timestamp.valueOf(val). However, if val contains the UTC suffix, Timestamp.valueOf will throw an IllegalArgumentException because it does not support timezone suffixes. To make the fallback robust, we should strip the UTC suffix from the fallback string.
try {
return Timestamp.from(Instant.parse(iso));
} catch (DateTimeParseException e) {
// Fallback for non-standard formats
String fallback = val;
if (fallback.endsWith(" UTC")) {
fallback = fallback.substring(0, fallback.length() - 4);
}
return Timestamp.valueOf(fallback);
}| if (zoneId == null) { | ||
| // JDBC 4.2 Modern API (no Calendar provided): | ||
| // Use pure JSR-310 math for perfectly accurate modern conversions without Calendar quirks. | ||
| return new Time( | ||
| localTime | ||
| .atDate(LocalDate.of(1970, 1, 1)) | ||
| .atZone(ZoneId.systemDefault()) | ||
| .toInstant() | ||
| .toEpochMilli()); | ||
| } |
There was a problem hiding this comment.
When converting or shifting timezone fields for JDBC types like Time, we should use legacy Calendar manipulation instead of java.time APIs to ensure Daylight Saving Time (DST) is handled properly, in accordance with the general rules.
| if (zoneId == null) { | |
| // JDBC 4.2 Modern API (no Calendar provided): | |
| // Use pure JSR-310 math for perfectly accurate modern conversions without Calendar quirks. | |
| return new Time( | |
| localTime | |
| .atDate(LocalDate.of(1970, 1, 1)) | |
| .atZone(ZoneId.systemDefault()) | |
| .toInstant() | |
| .toEpochMilli()); | |
| } | |
| if (zoneId == null) { | |
| // JDBC 4.2 Modern API (no Calendar provided): | |
| // Use legacy Calendar manipulation to ensure Daylight Saving Time (DST) is handled properly. | |
| Calendar targetCal = Calendar.getInstance(); | |
| targetCal.set(Calendar.YEAR, 1970); | |
| targetCal.set(Calendar.MONTH, Calendar.JANUARY); | |
| targetCal.set(Calendar.DAY_OF_MONTH, 1); | |
| targetCal.set(Calendar.HOUR_OF_DAY, localTime.getHour()); | |
| targetCal.set(Calendar.MINUTE, localTime.getMinute()); | |
| targetCal.set(Calendar.SECOND, localTime.getSecond()); | |
| targetCal.set(Calendar.MILLISECOND, localTime.getNano() / 1_000_000); | |
| return new Time(targetCal.getTimeInMillis()); | |
| } |
References
- When converting or shifting timezone fields for JDBC types like java.sql.Time, use legacy Calendar manipulation instead of java.time APIs to ensure Daylight Saving Time (DST) is handled properly.
| if (targetClass == LocalTime.class) { | ||
| // java.sql.Time.toLocalTime() drops milliseconds (JDK bug). | ||
| // We manually convert it using the JVM offset to preserve millisecond precision. | ||
| long epochMillis = sqlTime.getTime(); | ||
| return Instant.ofEpochMilli(epochMillis) | ||
| .atZone(ZoneId.systemDefault()) | ||
| .toLocalTime(); | ||
| } |
There was a problem hiding this comment.
When converting Time to LocalTime, we should avoid using the built-in Time.toLocalTime() method if millisecond precision must be preserved, as it truncates sub-second data. Instead, use a manual conversion that accounts for timezone offsets and utilizes LocalTime.ofNanoOfDay to maintain precision, in accordance with the general rules.
if (targetClass == LocalTime.class) {
// Time.toLocalTime() drops milliseconds (JDK bug).
// We manually convert it using the JVM offset and LocalTime.ofNanoOfDay to preserve precision.
long epochMillis = sqlTime.getTime();
long jvmOffsetMs = ZoneId.systemDefault().getRules().getOffset(Instant.ofEpochMilli(epochMillis)).getTotalSeconds() * 1000L;
long localMillis = (epochMillis + jvmOffsetMs) % 86400000L;
if (localMillis < 0) {
localMillis += 86400000L;
}
return LocalTime.ofNanoOfDay(localMillis * 1_000_000L);
}References
- When converting java.sql.Time to java.time.LocalTime, avoid using the built-in java.sql.Time.toLocalTime() method if millisecond precision must be preserved, as it truncates sub-second data. Instead, use a manual conversion that accounts for timezone offsets and utilizes LocalTime.ofNanoOfDay to maintain precision.
No description provided.