feat(bigquery): add timestamp precision support to schema #13421
Conversation
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 enhances the BigQuery client library by introducing support for specifying timestamp precision in table schemas. This allows users to define 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 adds support for timestamp precision in BigQuery schemas. The implementation is mostly correct and includes unit and integration tests. However, there is a significant issue with the current implementation that prevents setting a timestamp precision of 0, as it would be ignored and default to 6. I've suggested changing FieldSchema.TimestampPrecision to a pointer to fix this, and pointed out the necessary related changes. I've also left a couple of smaller suggestions to improve documentation clarity and the design of a new helper function.
| // base 10) for seconds of TIMESTAMP type. Possible values include: | ||
| // * 6 (Default, for TIMESTAMP type with microsecond precision) | ||
| // * 12 (For TIMESTAMP type with picosecond precision) | ||
| TimestampPrecision int64 |
There was a problem hiding this comment.
The TimestampPrecision field should be a pointer (*int64) to correctly handle the case where a user wants to explicitly set the precision to 0.
According to BigQuery documentation, timestamp precision can be a value from 0 to 12. If not specified, it defaults to 6. With the current int64 type, it's impossible to distinguish between an unset value and an explicit 0, leading to a bug where a user-specified precision of 0 is ignored and defaults to 6.
Changing this to *int64 will allow nil to represent "not set" and a pointer to 0 to represent an explicit 0. This is a breaking change but necessary for correctness. Corresponding changes will be needed in toBQ, bqToFieldSchema, and the tests.
| TimestampPrecision int64 | |
| TimestampPrecision *int64 |
| if fs.TimestampPrecision > 0 { | ||
| tfs.TimestampPrecision = &fs.TimestampPrecision | ||
| } |
There was a problem hiding this comment.
This logic is incorrect as it prevents setting TimestampPrecision to 0. Once FieldSchema.TimestampPrecision is changed to a pointer type (*int64) as suggested in another comment, this check should be updated to test for nil instead.
| if fs.TimestampPrecision > 0 { | |
| tfs.TimestampPrecision = &fs.TimestampPrecision | |
| } | |
| if fs.TimestampPrecision != nil { | |
| tfs.TimestampPrecision = fs.TimestampPrecision | |
| } |
| if tfs.TimestampPrecision != nil { | ||
| fs.TimestampPrecision = *tfs.TimestampPrecision | ||
| } |
There was a problem hiding this comment.
This assignment needs to be updated to work with *int64 for FieldSchema.TimestampPrecision as suggested in another comment. Instead of dereferencing the pointer, you should assign it directly.
| if tfs.TimestampPrecision != nil { | |
| fs.TimestampPrecision = *tfs.TimestampPrecision | |
| } | |
| if tfs.TimestampPrecision != nil { | |
| fs.TimestampPrecision = tfs.TimestampPrecision | |
| } |
| // Precision (maximum number of total digits in | ||
| // base 10) for seconds of TIMESTAMP type. Possible values include: |
There was a problem hiding this comment.
The description of Precision here is a bit confusing. BigQuery documentation for TIMESTAMP precision refers to the number of fractional digits for seconds. The current comment mentions "maximum number of total digits", which is how precision for NUMERIC types is usually described.
To avoid confusion, could you clarify that this is about fractional digits? For example:
// Precision is the number of fractional digits for seconds of a TIMESTAMP type.
| // Precision (maximum number of total digits in | |
| // base 10) for seconds of TIMESTAMP type. Possible values include: | |
| // Precision is the number of fractional digits for seconds of a | |
| // TIMESTAMP type. Possible values include: |
| // Utility functions to deal with byproducts of wrapper types in discovery. | ||
| func int64ptr(v int64) *int64 { return &v } |
There was a problem hiding this comment.
This helper function is only used in tests currently, but if FieldSchema.TimestampPrecision is changed to a pointer type as suggested in another comment, users of this library will need an exported helper to construct *int64 values.
Consider exporting this function (e.g., Int64p) and improving the comment to be more general.
Example:
// Int64p returns a pointer to an int64 value.
func Int64p(v int64) *int64 { return &v }| // Utility functions to deal with byproducts of wrapper types in discovery. | |
| func int64ptr(v int64) *int64 { return &v } | |
| // Int64p returns a pointer to an int64 value. | |
| func Int64p(v int64) *int64 { return &v } |
1 participant
towards internal b/447623020