feat(bigquery-jdbc): implement TypeRegistry and TypeDescriptor - #13947
feat(bigquery-jdbc): implement TypeRegistry and TypeDescriptor#13947Neenu1995 wants to merge 6 commits into
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.
| (val, zone) -> { | ||
| if (val instanceof Date) return (Date) val; | ||
| if (val instanceof LocalDate) return Date.valueOf((LocalDate) val); | ||
| throw new IllegalArgumentException("Cannot convert to DATE: " + val); |
There was a problem hiding this comment.
In JDBC, it is common to convert other date/time types (like java.sql.Timestamp or java.util.Date) to java.sql.Date. Adding support for java.util.Date (which is the superclass of java.sql.Timestamp) makes the driver much more robust and user-friendly.
if (val instanceof Date) return (Date) val;
if (val instanceof LocalDate) return Date.valueOf((LocalDate) val);
if (val instanceof java.util.Date) return new Date(((java.util.Date) val).getTime());
throw new IllegalArgumentException("Cannot convert to DATE: " + val);
}));| (val, zone) -> { | ||
| if (val instanceof Time) return (Time) val; | ||
| if (val instanceof LocalTime) return Time.valueOf((LocalTime) val); | ||
| throw new IllegalArgumentException("Cannot convert to TIME: " + val); |
There was a problem hiding this comment.
In JDBC, it is common to convert other date/time types (like java.sql.Timestamp or java.util.Date) to java.sql.Time. Adding support for java.util.Date makes the driver much more robust and user-friendly.
(val, zone) -> {
if (val instanceof Time) return (Time) val;
if (val instanceof LocalTime) return Time.valueOf((LocalTime) val);
if (val instanceof java.util.Date) return new Time(((java.util.Date) val).getTime());
throw new IllegalArgumentException("Cannot convert to TIME: " + val);| Types.OTHER, | ||
| String.class, | ||
| StandardSQLTypeName.JSON, | ||
| Arrays.asList(com.google.gson.JsonObject.class), |
There was a problem hiding this comment.
Registering com.google.gson.JsonElement.class instead of com.google.gson.JsonObject.class allows the type registry to automatically resolve any Gson JSON element (including JsonArray, JsonPrimitive, etc.) to the JSON descriptor, thanks to the fallback assignability check in getDescriptorForClass.
| Arrays.asList(com.google.gson.JsonObject.class), | |
| Arrays.asList(com.google.gson.JsonElement.class), |
| */ | ||
| 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.
|
|
||
| static { | ||
| int maxOrdinal = 0; | ||
| for (StandardSQLTypeName type : StandardSQLTypeName.values()) { |
There was a problem hiding this comment.
Isn't this same as StandardSQLTypeName.values().length()?
| } | ||
|
|
||
| private static void register(TypeDescriptor<?> descriptor) { | ||
| if (DESCRIPTORS_BY_ORDINAL[descriptor.getBqType().ordinal()] == null) { |
There was a problem hiding this comment.
Should we throw an exception if it already exists? That should not be possible
| return null; | ||
| } | ||
| TypeDescriptor<?> descriptor = getDescriptorForClass(targetClass); | ||
| if (descriptor == null) { |
There was a problem hiding this comment.
nit: move exception to getDescriptorForClass instead of returning null since that's incorrect?
|
|
||
| /** | ||
| * 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?
| Timestamp.class, | ||
| StandardSQLTypeName.DATETIME, | ||
| Arrays.asList(Timestamp.class, LocalDateTime.class), | ||
| (val, targetClass, zone) -> { |
There was a problem hiding this comment.
My suggestion is to move all these conversion methods from inline to standalone methods that we can reference here.
It will be easier to unittest & read.
2 participants
No description provided.