-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for INTERVAL data type to list_rows
#840
Conversation
Some of these commits are from #829 |
google/cloud/bigquery/_helpers.py
Outdated
minutes = time_sign * int(parsed.group("minutes")) | ||
seconds = time_sign * int(parsed.group("seconds")) | ||
fraction = parsed.group("fraction") | ||
microseconds = time_sign * int(fraction.ljust(6, "0")) if fraction else 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell from https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#interval_type if there are limits on any of these fields. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The docs don't really explain much about how this data format actually works. From what is documented and trial and error, I think these are the limits:
Min
- years: -10000
- months: -11 (I think?)
- days: -3660000
- hours: -87840000
- minutes: -59 (I think?)
- seconds: -59 (I think?)
- microseconds: -999999 (I think?)
Max
- years: 10000
- months: 11 (I think?)
- days: 3660000
- hours: 87840000
- minutes: 59 (I think?)
- seconds: 59 (I think?)
- microseconds: 999999 (I think?)
I don't think we'll need any client-side validation for this, but it does remind me that we should have some unit tests that exercise these limits.
google/cloud/bigquery/enums.py
Outdated
BOOL = ScalarQueryParameterType("BOOL") | ||
BOOLEAN = ScalarQueryParameterType("BOOL") | ||
BIGDECIMAL = ScalarQueryParameterType("BIGNUMERIC") | ||
BIGNUMERIC = ScalarQueryParameterType("BIGNUMERIC") | ||
BYTES = ScalarQueryParameterType("BYTES") | ||
INTEGER = ScalarQueryParameterType("INT64") | ||
INT64 = ScalarQueryParameterType("INT64") | ||
DATE = ScalarQueryParameterType("DATE") | ||
DATETIME = ScalarQueryParameterType("DATETIME") | ||
DECIMAL = ScalarQueryParameterType("NUMERIC") | ||
FLOAT = ScalarQueryParameterType("FLOAT64") | ||
FLOAT64 = ScalarQueryParameterType("FLOAT64") | ||
NUMERIC = ScalarQueryParameterType("NUMERIC") | ||
BIGNUMERIC = ScalarQueryParameterType("BIGNUMERIC") | ||
DECIMAL = ScalarQueryParameterType("NUMERIC") | ||
BIGDECIMAL = ScalarQueryParameterType("BIGNUMERIC") | ||
BOOLEAN = ScalarQueryParameterType("BOOL") | ||
BOOL = ScalarQueryParameterType("BOOL") | ||
GEOGRAPHY = ScalarQueryParameterType("GEOGRAPHY") | ||
TIMESTAMP = ScalarQueryParameterType("TIMESTAMP") | ||
DATE = ScalarQueryParameterType("DATE") | ||
INT64 = ScalarQueryParameterType("INT64") | ||
INTEGER = ScalarQueryParameterType("INT64") | ||
NUMERIC = ScalarQueryParameterType("NUMERIC") | ||
STRING = ScalarQueryParameterType("STRING") | ||
TIME = ScalarQueryParameterType("TIME") | ||
DATETIME = ScalarQueryParameterType("DATETIME") | ||
TIMESTAMP = ScalarQueryParameterType("TIMESTAMP") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to include INTERVAL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did but then reverted it because the query parameter support is incomplete. I could revert the alphabetization, but I figure we should do that at some point.
I think this is ready, but I'll keep the Do Not Merge label, as I think #829 should go in first. |
@plamut Did you have any additional thoughts on this? It'd be good to get in before v3. I added some proposed unit tests with trailing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went over this again, it looks fine (just one nit).
Good that we caught the issue in the divmod
-based implementation, keeping the regex is the right way to go.
* test: refactor `list_rows` tests and add test for scalars * WIP: INTERVAL support * feat: add support for INTERVAL data type to `list_rows` * fix relativedelta construction for non-microseconds * WIP: support INTERVAL query params * remove dead code * INTERVAL not supported in query parameters * revert query parameter changes * add validation error for interval * add unit tests for extreme intervals * add dateutil to intersphinx * use dictionary for intersphinx * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * add test case for trailing . * explicit none * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * truncate nanoseconds * use \d group for digits * use \d for consistency Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Peter Lamut <plamut@users.noreply.github.com>
Implementation of most other features for
INTERVAL
is blocked by backend changes.Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Towards #826 🦕