Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for handling null parameter values during bulk inserts using the BigQuery Write API by inserting JsonNull.INSTANCE into the row object, and adds a corresponding unit test. However, the changes bypass parameter binding during the dry run query in executeBatch() and remove the getWriteBatchJobConfiguration helper method. The reviewer correctly points out that this will cause production failures for parameterized queries, as BigQuery requires parameters to be bound or declared even during dry runs. It is recommended to restore parameter binding and the helper method.
| if (this.insertSchema == null && this.insertTableName == null) { | ||
| QueryStatistics insertJobQueryStatistics = | ||
| getQueryStatistics(getWriteBatchJobConfiguration(currentParameterList)); | ||
| getQueryStatistics(getJobConfig(this.currentQuery).build()); | ||
| setInsertMetadata(insertJobQueryStatistics); | ||
| } |
There was a problem hiding this comment.
Bypassing parameter binding for the dry run query by using getJobConfig(this.currentQuery).build() will cause executeBatch() to fail in production for any parameterized query (e.g., INSERT INTO table (col) VALUES (?)).
BigQuery requires all parameters to be bound or declared even during dry runs; otherwise, the backend will throw a BigQueryException indicating that a query parameter or placeholder was not found.
The unit test testExecuteBatchNullWithWriteAPI did not catch this because it mocks bigQuery.create(any(JobInfo.class)) to return a mocked job and statistics directly, bypassing the actual BigQuery dry run validation.
To fix this, we should retain the parameter binding for the dry run. If there was an issue with handling null values in BigQueryParameterHandler, we should address that specific issue within BigQueryParameterHandler or getWriteBatchJobConfiguration rather than removing parameter binding entirely.
| if (this.insertSchema == null && this.insertTableName == null) { | |
| QueryStatistics insertJobQueryStatistics = | |
| getQueryStatistics(getWriteBatchJobConfiguration(currentParameterList)); | |
| getQueryStatistics(getJobConfig(this.currentQuery).build()); | |
| setInsertMetadata(insertJobQueryStatistics); | |
| } | |
| ArrayList<BigQueryJdbcParameter> currentParameterList = this.batchParameters.peek(); | |
| if (this.insertSchema == null && this.insertTableName == null) { | |
| QueryStatistics insertJobQueryStatistics = | |
| getQueryStatistics(getWriteBatchJobConfiguration(currentParameterList)); | |
| setInsertMetadata(insertJobQueryStatistics); | |
| } |
|
|
||
| QueryJobConfiguration getWriteBatchJobConfiguration( | ||
| ArrayList<BigQueryJdbcParameter> currentParameterList) throws SQLException { | ||
| LOG.finer("++enter++"); | ||
| BigQueryParameterHandler batchHandler = | ||
| new BigQueryParameterHandler(this.parameterCount, currentParameterList); | ||
| QueryJobConfiguration.Builder jobConfiguration = getJobConfig(this.currentQuery); | ||
| jobConfiguration.setParameterMode("POSITIONAL"); | ||
| jobConfiguration = batchHandler.configureParameters(jobConfiguration); | ||
| return jobConfiguration.build(); | ||
| } | ||
|
|
||
| QueryJobConfiguration getStandardBatchJobConfiguration(String query) throws SQLException { |
There was a problem hiding this comment.
Please restore the getWriteBatchJobConfiguration helper method to support parameter binding during the dry run for the Write API path.
QueryJobConfiguration getWriteBatchJobConfiguration(
ArrayList<BigQueryJdbcParameter> currentParameterList) throws SQLException {
LOG.finer("++enter++");
BigQueryParameterHandler batchHandler =
new BigQueryParameterHandler(this.parameterCount, currentParameterList);
QueryJobConfiguration.Builder jobConfiguration = getJobConfig(this.currentQuery);
jobConfiguration.setParameterMode("POSITIONAL");
jobConfiguration = batchHandler.configureParameters(jobConfiguration);
return jobConfiguration.build();
}
QueryJobConfiguration getStandardBatchJobConfiguration(String query) throws SQLException {
No description provided.