refactor(jdbc): migrate Read Path to BigQueryTypeRegistry - #14063
refactor(jdbc): migrate Read Path to BigQueryTypeRegistry#14063Neenu1995 wants to merge 11 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 refactors type conversion and mapping across the BigQuery JDBC driver by replacing BigQueryTypeCoercer and BigQueryJdbcTypeMappings with BigQueryTypeRegistry. This change requires several methods to declare throwing SQLException. The review feedback suggests keeping the parameter type as FieldValue in BigQueryJsonStruct to avoid redundant downcasts, catching Exception instead of RuntimeException in BigQueryBaseResultSet to properly handle SQLException during conversion, and removing redundant blank lines in BigQueryTypeRegistry.
| Arrays.asList(String.class), | ||
| (val, targetClass, zone) -> String.valueOf(val)); | ||
| (val, targetClass, zone) -> { | ||
| if (val == null) return null; |
There was a problem hiding this comment.
please update with brackets instead of a single line if-return
| @@ -154,9 +207,16 @@ static TypeDescriptor<?> createDateDescriptor() { | |||
| // TODO(Phase 3): Add native JSR-310 fast-path to bypass boxing for LocalDate | |||
| Date sqlDate; | |||
| if (val instanceof Date) sqlDate = (Date) val; | |||
| @@ -154,9 +207,16 @@ static TypeDescriptor<?> createDateDescriptor() { | |||
| // TODO(Phase 3): Add native JSR-310 fast-path to bypass boxing for LocalDate | |||
| Date sqlDate; | |||
| if (val instanceof Date) sqlDate = (Date) val; | |||
| Object getCoercedValue(int index) throws SQLException { | ||
| LOG.finestTrace("getCoercedValue"); | ||
| Object value = this.values.get(index); | ||
| if (value instanceof Integer |
There was a problem hiding this comment.
Can this be moved to TypeRegistry too? We should avoid conversions outside of it
| public static Timestamp boxTimestamp(String val) { | ||
| // Check if the value is a numeric float string (e.g. "1680174859.8202269" from JSON API) | ||
| try { | ||
| if (val.indexOf('-') < 0 |
There was a problem hiding this comment.
Do we need this if check? There are a lot of other cases when it won't be a valid float number string. We can just validate the format or allow BigDecimal(val) to fail parsing
2 participants
This PR migrates the BigQuery JDBC read-path implementations (Arrow and Json) to utilize the newly introduced
BigQueryTypeRegistryfor unified type lookups and high-performance coercion.Also made the following fixes in implementation and tests:
Type Registry Consolidation & Read-Path Migration
BigQueryTypeCoercerwithBigQueryTypeRegistryas the single source of truth for all type casting and format mapping.BigQueryBaseResultSet,BigQueryArrowResultSet, andBigQueryJsonResultSet(along with their nested Struct and Array counterparts) to funnel allgetObject()and primitive getter calls through the registry.Temporal Type & JDBC 4.2 Spec Compliance
java.sql.Dateto strictly format at UTC midnight, andjava.sql.Timeto1970-01-01in the local JVM, ensuring correct String serialization and civil time preservation.BigQueryJdbcExceptionfor invalid cross-temporal conversions (e.g., blockingjava.sql.Date->java.sql.Timeandjava.sql.Time->java.sql.Date), preserving backward compatibility with legacy strictness.DateTimeFormattercompilations intostatic finalconstants to guarantee fast-path string serialization forTIMESTAMP(6 decimal precision) andTIME(3 decimal precision) without per-row object allocation penalties.Complex Types & Boundary Protection
Integer(days since epoch) andLong(microseconds since epoch) unboxing explicitly inside Arrow structures to bypass global registry mutations, fixing nestedRANGEandARRAYrendering.MAX_VALUE/MIN_VALUEguards) when coercingBigDecimalandLongvalues down to smaller primitives (Integer,Short,Byte) to prevent silent data corruption.