feat(bigquery-jdbc): implement TypeRegistry and TypeDescriptor - #13947
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces BigQueryTypeRegistry and TypeDescriptor to handle bidirectional type mapping and coercion between JDBC, Java, and BigQuery. The review feedback highlights several key issues and improvements: a potential runtime exception in the TIMESTAMP coercer for Java 8 date/time types, precision loss when converting Number to BigDecimal using doubleValue(), and unused code involving DESCRIPTORS_BY_ORDINAL. Additionally, suggestions were made to expand conversion support for java.util.Date and Gson's JsonElement to make the registry more robust.
| */ | ||
| final class BigQueryTypeRegistry { | ||
|
|
||
| private static final TypeDescriptor<?>[] DESCRIPTORS_BY_ORDINAL; |
There was a problem hiding this comment.
The DESCRIPTORS_BY_ORDINAL array is declared and populated but never read anywhere in the class. It should be removed to clean up unused code.
| int maxOrdinal = 0; | ||
| for (StandardSQLTypeName type : StandardSQLTypeName.values()) { | ||
| if (type.ordinal() > maxOrdinal) { | ||
| maxOrdinal = type.ordinal(); | ||
| } | ||
| } | ||
| DESCRIPTORS_BY_ORDINAL = new TypeDescriptor<?>[maxOrdinal + 1]; |
There was a problem hiding this comment.
The initialization of DESCRIPTORS_BY_ORDINAL is unused and can be removed.
| if (DESCRIPTORS_BY_ORDINAL[descriptor.getBqType().ordinal()] == null) { | ||
| DESCRIPTORS_BY_ORDINAL[descriptor.getBqType().ordinal()] = descriptor; | ||
| } |
There was a problem hiding this comment.
The registration of DESCRIPTORS_BY_ORDINAL is unused and can be removed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a type mapping and coercion system between JDBC, Java, and BigQuery types, consisting of BigQueryTimezoneUtility, BigQueryTypeRegistry, and TypeDescriptor. The feedback highlights several key areas for improvement: resolving a potential parsing bug in boxTimestamp for strings with timezone offsets, using legacy Calendar manipulation for java.sql.Time to handle DST correctly, preserving millisecond precision when converting to LocalTime, and removing the unused DESCRIPTORS_BY_ORDINAL array.
|
|
||
| /** | ||
| * Returns the exact BigQuery StandardSQLTypeName for a given Java class. If no mapping is found, | ||
| * returns StandardSQLTypeName.STRING as a fallback. |
There was a problem hiding this comment.
Why do we need a fallback? Shouldn't we implement all possible conversions?
There was a problem hiding this comment.
We need the STRING fallback to preserve backward compatibility and prevent crashing legacy applications that rely on the driver's old behavior of silently stringifying unmapped objects so the BigQuery backend can parse them.
|
/gcprun |
| * High-performance hotpath convert for ResultSets. Converts the input value using the default | ||
| * mapping for the given BigQuery type via O(1) array indexing. | ||
| */ | ||
| public static Object convert(Object input, StandardSQLTypeName bqType, ZoneId zoneId) |
There was a problem hiding this comment.
nit: I'd add overrides without ZoneId. It is used only with date/time objects, but we have a lot of other conversions where zone is unused.
|
|
||
| /** | ||
| * Converts a BigQuery civil DATE string into an absolute Date by anchoring it to midnight of the | ||
| * provided timezone (or JVM default if null). |
There was a problem hiding this comment.
So this is where I'm not 100% sure about the behavior.
We have 4 time-related types: timestamp, time, date, datetime.
Time, Date, Datetime are not timezone specific, so we should be reading them as-is. If it is stored as "12:30:00", it should remain "12:30:00" regardless of the JVM timezone or provided timezone.
Timestamp is the only value that needs to be adjusted to the timezone.
There was a problem hiding this comment.
java.sql.Datestores epoch millis under the hood, not civil year/month/day fields.getDate(col, cal)contract: Per JDBC spec, the returnedjava.sql.Datemust represent midnight (00:00:00) in the providedCalendar's timezone.- Prevents date shifting: Without anchoring to midnight in the target timezone, formatting or reading the date in that timezone would shift it to the wrong day (e.g., UTC midnight
2026-07-17displays as2026-07-16 20:00in New York time).
(Note: Modern LocalDate via getObject(col, LocalDate.class) is already timezone-agnostic and read as-is without any conversion).
| Returned Class / Method | Underlying Representation | Timezone Handling |
|---|---|---|
getObject(col, LocalDate.class) |
java.time.LocalDate |
As-is (completely timezone-agnostic) |
getObject(col, LocalTime.class) |
java.time.LocalTime |
As-is (completely timezone-agnostic) |
getDate(col) |
java.sql.Date |
Anchored to 00:00:00 in JVM default timezone |
getDate(col, Calendar cal) |
java.sql.Date |
Anchored to 00:00:00 in target Calendar timezone |
getTimestamp(col, Calendar cal) |
java.sql.Timestamp |
Absolute instant adjusted to target Calendar timezone |
2 participants
No description provided.