Skip to content

feat: BigQueryTypeRegistry core matrix (Phase 1) - #13951

Closed
Neenu1995 wants to merge 7 commits into
mainfrom
feature/type-registry-phase-1
Closed

feat: BigQueryTypeRegistry core matrix (Phase 1)#13951
Neenu1995 wants to merge 7 commits into
mainfrom
feature/type-registry-phase-1

Conversation

@Neenu1995

Copy link
Copy Markdown
Contributor

No description provided.

@Neenu1995
Neenu1995 requested review from a team as code owners July 29, 2026 19:58

@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 a new type-handling and conversion engine for the BigQuery JDBC driver, adding the BigQueryTimezoneUtility, BigQueryTypeRegistry, and TypeDescriptor classes. The review feedback highlights several critical improvements and bug fixes: handling the ' UTC' suffix in timestamp parsing to avoid exceptions, adding bounds checks to prevent silent truncation when downcasting INT64 values, using legacy Calendar manipulation for proper DST handling in time conversions, preventing a potential NullPointerException in the ConcurrentHashMap lookup, and optimizing type conversion with a fast-path check.

Comment on lines +73 to +78
public static Timestamp boxTimestamp(String val) {
if (val.contains("T") || val.contains("Z")) {
return Timestamp.from(Instant.parse(val));
}
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

BigQuery's canonical string representation for absolute TIMESTAMP values ends with a UTC suffix (e.g., 2014-08-19 12:41:35.220000 UTC). Calling Timestamp.valueOf on such strings will throw an IllegalArgumentException because it does not support timezone suffixes. We should explicitly handle the UTC suffix by converting it to an ISO-8601 UTC format before parsing.

  public static Timestamp boxTimestamp(String val) {
    if (val.endsWith(" UTC")) {
      String isoString = val.substring(0, val.length() - 4).replace(' ', 'T') + 'Z';
      return Timestamp.from(Instant.parse(isoString));
    }
    if (val.contains("T") || val.contains("Z")) {
      return Timestamp.from(Instant.parse(val));
    }
    return Timestamp.valueOf(val);
  }

Comment on lines +88 to +98
(val, targetClass, zone) -> {
long longVal;
if (val instanceof Number) longVal = ((Number) val).longValue();
else if (val instanceof String) longVal = Long.parseLong((String) val);
else throw new BigQueryJdbcException("Cannot convert to INT64: " + val);

if (targetClass == Integer.class) return (int) longVal;
if (targetClass == Short.class) return (short) longVal;
if (targetClass == Byte.class) return (byte) longVal;
return longVal;
}));

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

When converting an INT64 (represented as a long) to smaller integer types (Integer, Short, Byte), casting directly can cause silent truncation or overflow. We should perform bounds checks and throw a BigQueryJdbcException if the value is out of range for the target type.

            (val, targetClass, zone) -> {
              long longVal;
              if (val instanceof Number) longVal = ((Number) val).longValue();
              else if (val instanceof String) longVal = Long.parseLong((String) val);
              else throw new BigQueryJdbcException("Cannot convert to INT64: " + val);

              if (targetClass == Integer.class) {
                if (longVal < Integer.MIN_VALUE || longVal > Integer.MAX_VALUE) {
                  throw new BigQueryJdbcException("Loss of precision converting " + longVal + " to Integer");
                }
                return (int) longVal;
              }
              if (targetClass == Short.class) {
                if (longVal < Short.MIN_VALUE || longVal > Short.MAX_VALUE) {
                  throw new BigQueryJdbcException("Loss of precision converting " + longVal + " to Short");
                }
                return (short) longVal;
              }
              if (targetClass == Byte.class) {
                if (longVal < Byte.MIN_VALUE || longVal > Byte.MAX_VALUE) {
                  throw new BigQueryJdbcException("Loss of precision converting " + longVal + " to Byte");
                }
                return (byte) longVal;
              }
              return longVal;
            }));

Comment on lines +59 to +67
public static Time boxTime(String val, ZoneId zoneId) {
ZoneId targetZone = zoneId != null ? zoneId : ZoneId.systemDefault();
return new Time(
LocalTime.parse(val)
.atDate(LocalDate.of(1970, 1, 1))
.atZone(targetZone)
.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 java.sql.Time, use legacy Calendar manipulation instead of java.time APIs to ensure Daylight Saving Time (DST) is handled properly.

Suggested change
public static Time boxTime(String val, ZoneId zoneId) {
ZoneId targetZone = zoneId != null ? zoneId : ZoneId.systemDefault();
return new Time(
LocalTime.parse(val)
.atDate(LocalDate.of(1970, 1, 1))
.atZone(targetZone)
.toInstant()
.toEpochMilli());
}
public static Time boxTime(String val, ZoneId zoneId) {
ZoneId targetZone = zoneId != null ? zoneId : ZoneId.systemDefault();
LocalTime localTime = LocalTime.parse(val);
java.util.Calendar cal = java.util.Calendar.getInstance(java.util.TimeZone.getTimeZone(targetZone.getId()));
cal.clear();
cal.set(1970, 0, 1, localTime.getHour(), localTime.getMinute(), localTime.getSecond());
cal.set(java.util.Calendar.MILLISECOND, localTime.getNano() / 1_000_000);
return new Time(cal.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 +48 to +55
static {
int maxOrdinal = 0;
for (StandardSQLTypeName type : StandardSQLTypeName.values()) {
if (type.ordinal() > maxOrdinal) {
maxOrdinal = type.ordinal();
}
}
DESCRIPTORS_BY_ORDINAL = new TypeDescriptor<?>[maxOrdinal + 1];

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

Since enum ordinals are 0-indexed and sequential, the maximum ordinal is guaranteed to be StandardSQLTypeName.values().length - 1. We can simplify the array size calculation and avoid the loop entirely.

Suggested change
static {
int maxOrdinal = 0;
for (StandardSQLTypeName type : StandardSQLTypeName.values()) {
if (type.ordinal() > maxOrdinal) {
maxOrdinal = type.ordinal();
}
}
DESCRIPTORS_BY_ORDINAL = new TypeDescriptor<?>[maxOrdinal + 1];
static {
DESCRIPTORS_BY_ORDINAL = new TypeDescriptor<?>[StandardSQLTypeName.values().length];

Comment on lines +259 to +268
public static <T> T convert(Object input, Class<T> targetClass) throws BigQueryJdbcException {
if (input == null) {
return null;
}
TypeDescriptor<?> descriptor = getDescriptorForClass(targetClass);
if (descriptor == null) {
throw new BigQueryJdbcException("Unsupported target class: " + targetClass.getName());
}
return (T) descriptor.convert(input, targetClass, null);
}

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

We can optimize the convert method by adding a fast-path check: if the input is already an instance of the target class, we can return it directly. This avoids map lookups, descriptor retrieval, and coercion overhead.

  public static <T> T convert(Object input, Class<T> targetClass) throws BigQueryJdbcException {
    if (input == null) {
      return null;
    }
    if (targetClass.isInstance(input)) {
      return targetClass.cast(input);
    }
    TypeDescriptor<?> descriptor = getDescriptorForClass(targetClass);
    if (descriptor == null) {
      throw new BigQueryJdbcException("Unsupported target class: " + targetClass.getName());
    }
    return (T) descriptor.convert(input, targetClass, null);
  }

Comment on lines +301 to +302
private static TypeDescriptor<?> getDescriptorForClass(Class<?> clazz) {
TypeDescriptor<?> descriptor = DESCRIPTORS_BY_CLASS.get(clazz);

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

Since DESCRIPTORS_BY_CLASS is a ConcurrentHashMap, calling get(null) will throw a NullPointerException. We should add a null check for clazz at the beginning of getDescriptorForClass to safely return null instead.

Suggested change
private static TypeDescriptor<?> getDescriptorForClass(Class<?> clazz) {
TypeDescriptor<?> descriptor = DESCRIPTORS_BY_CLASS.get(clazz);
private static TypeDescriptor<?> getDescriptorForClass(Class<?> clazz) {
if (clazz == null) {
return null;
}
TypeDescriptor<?> descriptor = DESCRIPTORS_BY_CLASS.get(clazz);

@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