Skip to content

feat: BigQueryTemporalUtility and temporal wiring (Phase 2) - #13952

Closed
Neenu1995 wants to merge 1 commit into
feature/type-registry-phase-1from
feature/temporal-utility-phase-2
Closed

feat: BigQueryTemporalUtility and temporal wiring (Phase 2)#13952
Neenu1995 wants to merge 1 commit into
feature/type-registry-phase-1from
feature/temporal-utility-phase-2

Conversation

@Neenu1995

Copy link
Copy Markdown
Contributor

No description provided.

@Neenu1995
Neenu1995 requested review from a team as code owners July 29, 2026 20:10

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +108 to +113
try {
return Timestamp.from(Instant.parse(iso));
} catch (java.time.format.DateTimeParseException e) {
// Fallback for non-standard formats
return Timestamp.valueOf(val);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);
    }

Comment on lines +66 to +75
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());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. 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.

Comment on lines +228 to +235
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();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. 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.

@Neenu1995 Neenu1995 closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant