refactor(jdbc): migrate Read Path to BigQueryTypeRegistry - #14063
refactor(jdbc): migrate Read Path to BigQueryTypeRegistry#14063Neenu1995 wants to merge 4 commits into
Conversation
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.
| private Object getValue(Field currentSchema, Object currentValue) throws SQLException { | ||
| LOG.finestTrace("getValue"); | ||
| if (isArray(currentSchema)) { | ||
| return new BigQueryJsonArray(currentSchema, currentValue, this.LOG.getJsonArrayLogger()); | ||
| return new BigQueryJsonArray( | ||
| currentSchema, (FieldValue) currentValue, this.LOG.getJsonArrayLogger()); | ||
| } else if (isStruct(currentSchema)) { | ||
| return new BigQueryJsonStruct( | ||
| currentSchema.getSubFields(), currentValue, this.LOG.getJsonStructLogger()); | ||
| currentSchema.getSubFields(), (FieldValue) currentValue, this.LOG.getJsonStructLogger()); | ||
| } else { | ||
| Class<?> targetClass = | ||
| BigQueryJdbcTypeMappings.standardSQLToJavaTypeMapping.get( | ||
| currentSchema.getType().getStandardType()); | ||
| return BIGQUERY_TYPE_COERCER.coerceTo(targetClass, currentValue, this.LOG); | ||
| return BigQueryTypeRegistry.convert( | ||
| currentValue, currentSchema.getType().getStandardType(), null); | ||
| } | ||
| } |
There was a problem hiding this comment.
By keeping the parameter type of currentValue as FieldValue (as it was originally), we can avoid the need for explicit downcasts to FieldValue on lines 72 and 75. Since FieldValue is an Object, it can still be passed directly to BigQueryTypeRegistry.convert without any issues.
| private Object getValue(Field currentSchema, Object currentValue) throws SQLException { | |
| LOG.finestTrace("getValue"); | |
| if (isArray(currentSchema)) { | |
| return new BigQueryJsonArray(currentSchema, currentValue, this.LOG.getJsonArrayLogger()); | |
| return new BigQueryJsonArray( | |
| currentSchema, (FieldValue) currentValue, this.LOG.getJsonArrayLogger()); | |
| } else if (isStruct(currentSchema)) { | |
| return new BigQueryJsonStruct( | |
| currentSchema.getSubFields(), currentValue, this.LOG.getJsonStructLogger()); | |
| currentSchema.getSubFields(), (FieldValue) currentValue, this.LOG.getJsonStructLogger()); | |
| } else { | |
| Class<?> targetClass = | |
| BigQueryJdbcTypeMappings.standardSQLToJavaTypeMapping.get( | |
| currentSchema.getType().getStandardType()); | |
| return BIGQUERY_TYPE_COERCER.coerceTo(targetClass, currentValue, this.LOG); | |
| return BigQueryTypeRegistry.convert( | |
| currentValue, currentSchema.getType().getStandardType(), null); | |
| } | |
| } | |
| private Object getValue(Field currentSchema, FieldValue currentValue) throws SQLException { | |
| LOG.finestTrace("getValue"); | |
| if (isArray(currentSchema)) { | |
| return new BigQueryJsonArray( | |
| currentSchema, currentValue, this.LOG.getJsonArrayLogger()); | |
| } else if (isStruct(currentSchema)) { | |
| return new BigQueryJsonStruct( | |
| currentSchema.getSubFields(), currentValue, this.LOG.getJsonStructLogger()); | |
| } else { | |
| return BigQueryTypeRegistry.convert( | |
| currentValue, currentSchema.getType().getStandardType(), null); | |
| } | |
| } |
| } catch (RuntimeException e) { | ||
| throw createCoercionException(columnIndex, type, e); | ||
| } |
There was a problem hiding this comment.
Since BigQueryTypeRegistry.convert throws SQLException, catching only RuntimeException here will bypass this catch block when a conversion error occurs. Catching Exception ensures that both RuntimeException and SQLException are caught and properly wrapped with the column context via createCoercionException.
| } catch (RuntimeException e) { | |
| throw createCoercionException(columnIndex, type, e); | |
| } | |
| } catch (Exception e) { | |
| throw createCoercionException(columnIndex, type, e); | |
| } |
| return String.class; | ||
|
|
||
| } | ||
| return DESCRIPTORS_BY_ORDINAL[ordinal].getDefaultJavaClass(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove the redundant blank lines introduced inside the if block and after the method to keep the code clean and consistent with standard formatting.
| return String.class; | |
| } | |
| return DESCRIPTORS_BY_ORDINAL[ordinal].getDefaultJavaClass(); | |
| } | |
| return String.class; | |
| } | |
| return DESCRIPTORS_BY_ORDINAL[ordinal].getDefaultJavaClass(); | |
| } | |
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.