feat(bigquery): transition format options #13422
Conversation
This PR is minimal, and updates all callsites that use the older UseInt64Timestamp boolean for controlling output to instead use the newer TimestampOutputFormat option and specify it using INT64. Followup PRs will update this to use the new ISO string for output.
product-auto-label
bot
added
the
api: bigquery
Summary of ChangesHello @shollyman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the BigQuery client library by transitioning its timestamp output formatting mechanism. It replaces the older boolean flag Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 correctly transitions from the deprecated UseInt64Timestamp boolean to the TimestampOutputFormat option for BigQuery calls, setting it to "INT64". The changes are applied consistently across all relevant call sites and tests. My main feedback is to define the string "INT64" as a constant to avoid magic strings and improve maintainability. This is particularly relevant as a follow-up PR is planned to change this value, and using a constant would make that change safer and more explicit. I've left comments on each occurrence.
| } | ||
| call := src.t.c.bqs.Tabledata.List(src.t.ProjectID, src.t.DatasetID, src.t.TableID) | ||
| call = call.FormatOptionsUseInt64Timestamp(true) | ||
| call = call.FormatOptionsTimestampOutputFormat("INT64") |
There was a problem hiding this comment.
To improve maintainability and avoid using a "magic string", consider defining "INT64" as a constant. This string appears in multiple files in this PR. Creating a constant, for example const timestampOutputFormatInt64 = "INT64", in a suitable shared location would make the code more readable and easier to update. The PR description mentions a future change to an ISO string format, which would be simpler and safer to implement with a constant.
| projectedFields := []googleapi.Field{"rows", "pageToken", "totalRows"} | ||
| call := src.j.c.bqs.Jobs.GetQueryResults(src.j.projectID, src.j.jobID).Location(src.j.location).Context(ctx) | ||
| call = call.FormatOptionsUseInt64Timestamp(true) | ||
| call = call.FormatOptionsTimestampOutputFormat("INT64") |
There was a problem hiding this comment.
As with the other call sites, replacing the magic string "INT64" with a shared constant would improve maintainability.
| // Use GetQueryResults only to wait for completion, not to read results. | ||
| call := j.c.bqs.Jobs.GetQueryResults(projectID, j.jobID).Location(j.location).Context(ctx).MaxResults(0) | ||
| call = call.FormatOptionsUseInt64Timestamp(true) | ||
| call = call.FormatOptionsTimestampOutputFormat("INT64") |
There was a problem hiding this comment.
Consider using a constant for "INT64" to avoid magic strings and improve code clarity and maintainability. This is especially relevant given the planned future changes to this format option.
| Labels: q.Labels, | ||
| FormatOptions: &bq.DataFormatOptions{ | ||
| UseInt64Timestamp: true, | ||
| TimestampOutputFormat: "INT64", |
There was a problem hiding this comment.
The string literal "INT64" is used in multiple places. It would be better to define this as a constant to improve maintainability and prevent potential typos. This also makes it easier to change the format in the future, as mentioned in the PR description.
| UseLegacySql: &pfalse, | ||
| FormatOptions: &bq.DataFormatOptions{ | ||
| UseInt64Timestamp: true, | ||
| TimestampOutputFormat: "INT64", |
There was a problem hiding this comment.
To ensure consistency and improve maintainability, this test should also use the proposed constant for "INT64" instead of a magic string.
| UseQueryCache: &pfalse, | ||
| FormatOptions: &bq.DataFormatOptions{ | ||
| UseInt64Timestamp: true, | ||
| TimestampOutputFormat: "INT64", |
There was a problem hiding this comment.
Similar to the other test case, using a constant for "INT64" here will make the test suite more robust to future changes of this format option.
1 participant
This PR is minimal, and updates all callsites that use the older UseInt64Timestamp boolean for controlling output to instead use the newer TimestampOutputFormat option and specify it using INT64.
Followup PRs will update this to use the new ISO string for output.
Towards internal b/447623020