impl(bigquery): add support for Interval type - #6137
Conversation
product-auto-label
Bot
added
the
api: bigquery
![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 introduces support for BigQuery INTERVAL values by adding a new Interval struct, implementing FromSql for it, and updating the time format constants to support unpadded values. It also refactors time parsing into a helper function and adds comprehensive unit and integration tests. The review feedback suggests deriving Copy and Eq on the Interval struct to improve API usability, and optimizing the parsing logic by using iterators instead of collecting into a Vec to avoid unnecessary heap allocations.
| } | ||
|
|
||
| /// Represents a BigQuery TIME INTERVAL value. | ||
| #[derive(Clone, Debug, Default, PartialEq)] |
There was a problem hiding this comment.
Since Interval only contains i32 fields, it is highly recommended to derive Copy and Eq to improve API usability and allow it to be easily copied and compared.
| #[derive(Clone, Debug, Default, PartialEq)] | |
| #[derive(Copy, Clone, Debug, Default, Eq, PartialEq)] |
References
- Implement Debug for all public types. Implement Default where appropriate. (API Design) (link)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6137 +/- ##
==========================================
- Coverage 96.54% 96.53% -0.01%
==========================================
Files 264 264
Lines 66072 66138 +66
==========================================
+ Hits 63789 63848 +59
- Misses 2283 2290 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return Err(ConvertError::Convert( | ||
| format!("invalid interval format: expected 3 parts, got {count}") | ||
| .into(), |
There was a problem hiding this comment.
I think displaying {s} would be more helpful than {count}
| } | ||
| } | ||
|
|
||
| /// Represents a BigQuery TIME INTERVAL value. |
There was a problem hiding this comment.
consider linking to https://docs.cloud.google.com/bigquery/docs/reference/standard-sql/data-types#interval_type
and (not in this PR) maybe do a pass on the other non-trivial types?
|
|
||
| /// Represents a BigQuery TIME INTERVAL value. | ||
| #[derive(Clone, Debug, Default, PartialEq)] | ||
| pub struct Interval { |
There was a problem hiding this comment.
thought for later: might be nice to have conversion helpers for wkt::Duration.
another thought for later: are we going to impl mathy traits like Add: https://doc.rust-lang.org/std/ops/trait.Add.html
I will answer my own questions and say: let's not do this any time soon. It is a trivial task to do later if we are asked to.
2 participants
Towards #5844