Skip to content

fix(bigquery-jdbc): add proper version to BigQueryDriver#13294

Merged
logachev merged 8 commits into
mainfrom
kirl/fix_version
Jun 25, 2026
Merged

fix(bigquery-jdbc): add proper version to BigQueryDriver#13294
logachev merged 8 commits into
mainfrom
kirl/fix_version

Conversation

@logachev

Copy link
Copy Markdown
Contributor

Update BigQueryDriver getMinorVersion/getMajorVersion to return proper value

@logachev logachev requested review from a team as code owners May 28, 2026 20:25

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the JDBC driver version loading and parsing logic by moving it from BigQueryDatabaseMetaData into a new utility class, BigQueryJdbcVersionUtility, which is then used by both BigQueryDatabaseMetaData and BigQueryDriver. A critical review comment points out a concurrency race condition in the lazy initialization of the utility class, which could lead to threads reading uninitialized major or minor versions. Additionally, the comment highlights a bug where single-part version strings would prevent the major version from being set. The reviewer suggests resolving these issues by using a thread-safe static initializer block to load the properties.

@keshavdandeva

Copy link
Copy Markdown
Contributor

Can we also update the README in this PR for the maven link

@logachev

logachev commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the JDBC driver version loading and parsing logic by extracting it from BigQueryDatabaseMetaData into a new utility class, BigQueryJdbcVersionUtility. Both BigQueryDatabaseMetaData and BigQueryDriver are updated to delegate version retrieval to this utility. Feedback on the new utility class points out potential thread contention from the synchronized ensureLoaded() method, risk of runtime exceptions, and a race condition during initialization. A more robust implementation using explicit locks and safe default fallbacks is suggested.

logachev added 3 commits June 25, 2026 15:08
This method was refactored into BigQueryJdbcVersionUtility but was
incorrectly kept during a merge conflict resolution, causing compilation
errors due to missing fields.

TAG=agy
CONV=93eab217-ad90-45ea-8b0b-dbfaea52887d
@logachev

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors how the JDBC driver version is loaded and managed by introducing a new utility class, BigQueryJdbcVersionUtility, which centralizes the loading of version properties from dependencies.properties. This replaces the duplicated loading logic in BigQueryDatabaseMetaData and hardcoded version constants in BigQueryDriver. The review feedback highlights potential performance issues due to synchronization overhead in ensureLoaded(), a possible regression where getDriverVersion() could return null instead of a fallback default version, and redundant concurrency primitives. A static initializer block is suggested to load the properties once during class loading, resolving these issues.

@logachev logachev enabled auto-merge (squash) June 25, 2026 23:07
@logachev logachev merged commit 9fd84fc into main Jun 25, 2026
194 of 195 checks passed
@logachev logachev deleted the kirl/fix_version branch June 25, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants