Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#840feat: add support for INTERVAL data type to
list_rows
#840Changes from 9 commits
5704f1e
5eb3794
9e54277
a89c1c1
60d9ca7
da6ef5b
b73a610
52f2b7b
ca8872e
018a617
2872d85
31c3f92
e3a3a6a
5f78311
bb03618
f3711e7
68035ba
9a011e9
9f6b02d
7cccbd2
152e8c2
f0f3fbd
c4636fa
18aae17
eccea82
5cdeffb
0497b19
92f41b9
0318f54
6b1f238
54e47f7
ced356b
dcc8b57
5c31fe2
2091478
21a2975
87b0d81
7bd48be
d62b950
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is a lone dot without fractional digits for microseconds possible? In other words, do we accept
H:M:S.
and interpret the missing digits as zero? (which is what Python does,123.
is acceptable form of123.0
).If not, it might make more sense to treat the
.F
part as an atomic optional part, because in the current from, the regex hints that the time part can also beH:M:SF
, but of course the seconds part will eat the F part, too, since we don't limit the number of matching digits.Do we know yet what the limits on the number if digits are? I assume the backend does not allow multiple representations of the same interval? (e.g. 125 seconds cannot be expressed as
0:0:125
, but only as0:2:5
)(although
relativedelta
can handle that just fine)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.
Actually, that regex has a bigger problem,
fraction
can be present without the dot -- well not really, but it kind of looks like that at first glance and makes me think too hard :).I think
r"(?P<seconds>[0-9]+)(?P<fraction>[.][0-9]*)?
would be better.
Or even:
r"(?P<seconds>[0-9]+(?:[.][0-9]*)?)
then something like (ignoring the time sign):
which seems simpler to me than the current logic.
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.
Indeed, that was the main point, perhaps I expressed it in a too convoluted way, sorry. :)
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.
divmod
looks elegant, but I found a rounding problem when I attempted to use this implementation. :-( googleapis/python-db-dtypes-pandas#18There 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.
When sending data, it seemed to allow it. But when getting data from the API, it always normalized it.
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
Max
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.
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.