refactor(bigquery-jdbc): migrate write path and metadata to TypeRegistry - #14061
refactor(bigquery-jdbc): migrate write path and metadata to TypeRegistry#14061Neenu1995 merged 6 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request refactors the BigQuery JDBC driver by migrating type mapping logic from BigQueryJdbcTypeMappings to BigQueryTypeRegistry across several classes, including BigQueryCallableStatement, BigQueryParameterMetaData, and BigQueryResultSetMetadata. The review feedback highlights several critical improvement opportunities: correcting misplaced Javadoc comments in BigQueryTypeRegistry, resolving dead code caused by a redundant null check on a primitive return value in BigQueryParameterMetaData, and addressing lossy type conversions where converting to a JDBC type first before mapping to a Java class loses precision for types like GEOGRAPHY or JSON. It is recommended to introduce a direct toJavaClass(StandardSQLTypeName) method to preserve type safety and prevent potential null pointer exceptions.
Sorry, something went wrong.
|
/gemini review |
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request refactors the BigQuery JDBC driver's type mapping mechanism by migrating from BigQueryJdbcTypeMappings to a centralized BigQueryTypeRegistry. This change affects several classes, including BigQueryCallableStatement, BigQueryParameterMetaData, and BigQueryResultSetMetadata. The review feedback highlights several areas for improvement: first, expanding the JDBC_TO_JAVA_CLASS_MAP to include missing standard JDBC types (such as DECIMAL and CHAR) to prevent runtime exceptions; and second, removing redundant null checks and boxing operations across the codebase, as the newly introduced registry methods either return primitive types or are guaranteed to return non-null values.
Sorry, something went wrong.
| return descriptor.getDefaultJavaClass(); | ||
| /** Returns the JDBC Type constant for a given BigQuery type. */ | ||
| public static int toJdbcType(StandardSQLTypeName bqType) { | ||
| if (bqType == null) return java.sql.Types.OTHER; |
There was a problem hiding this comment.
please avoid using one-line if-return, it is more difficult to read compared to
if (){
return;
}
Sorry, something went wrong.
|
|
||
| /** Returns the exact default Java Class for a given BigQuery type, avoiding lossy mappings. */ | ||
| public static Class<?> toJavaClass(StandardSQLTypeName bqType) { | ||
| if (bqType == null) return String.class; |
There was a problem hiding this comment.
Same as above
Sorry, something went wrong.
b/538177258
This PR executes Part 1 of Phase 4 of the Type Registry consolidation.
It migrates all parameter binding (Write Path) and metadata sizing away from the legacy `BigQueryJdbcTypeMappings` and onto the unified `BigQueryTypeRegistry`.
Changes: