-
Notifications
You must be signed in to change notification settings - Fork 384
feat: Add the interval type to Spanner #14254
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
base: main
Are you sure you want to change the base?
Conversation
c8f260b
to
68b6673
Compare
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've just reviewed the Interval type itself on this round.
Let's get that right before going into the rest.
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
namespace Google.Cloud.Spanner.V1 | ||
{ | ||
/// <summary> | ||
/// Representation of a Time 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.
We probably need more docs here. A link to Spanner's Interval type docs is probably 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.
Gentle ping on this one.
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.
Huh I thought the Spanner Interval type was not ready, but perhaps I am mistaken.
Is this a case of referencing something specifically?
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.
Nope, you are absolutely right. Maybe leave a TODO on your personal notes to come back here and add a link to the docs when they are public.
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
decimal nanoseconds = decimal.Parse(seconds) * NanosecondsInASecond; | ||
|
||
return (BigInteger) nanoseconds; |
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.
This is lossy, is that acceptable?
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.
To my testing this is the easier way to make this conversion. On my first implementations before actually making this draft, using FLOAT was giving me rounding errors, but after several iterations I landed on this solution.
If we multiply the decimal type by NanosecondsInASecond
is basically turning it into an int and removing the decimal point. Another solution I can think of is doing a direct String manipulation to have this value turn into a BigInteger? Removing the decimal point and adding as many 0s as needed for the conversion?
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
e0f35d5
to
dd7052b
Compare
/// P[n]Y[n]M[n]DT[n]H[n]M[n[.fraction]]S | ||
/// where [n] is an int. | ||
/// </summary> | ||
public static Interval Parse(string intervalString) |
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.
@Hectorhammett see how this looks like. No Regex and I think it's pretty clear.
It's a console app so it needs tweaking to be used here, and I stopped at the point of extracting each of the parts, that is I didn't combined the parts in the 3 fields months, days and nanos, but that should be easy enough.
Code here: https://gist.github.com/amanda-tarafa/af0f21b45c7a461fcefd11a3e6ca8136
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.
Yes, I like this approach.
My NodaTime code doing similar work is here, but Amanda's is cleaner, I think.
https://github.com/nodatime/nodatime/blob/main/src/NodaTime/Text/PeriodPattern.cs#L215
(It's possible that I don't detect a period ending in 'T' though - I need to check that at some point.)
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 like this one over the regex as well. Let me take a jab at it and run the tests and will update the code accordingly :)
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.
Most important changes are for missing tests. The rest is more around style and idiomatic coding, etc.
Use the protobuf pr as a reference: #13189
But we need to add this type to:
- AllTypesTableFixture
- BindingTests
- GetSchemaTableTests
- WriteTests
- SpannerParameterTest
A lot of these are integration tests that we couldn't have run until now, anyway.
namespace Google.Cloud.Spanner.V1 | ||
{ | ||
/// <summary> | ||
/// Representation of a Time 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.
Gentle ping on this one.
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/Interval.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDbType.ValueConversion.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDbType.StringParsing.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDbType.StringParsing.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerDbTypeTests.cs
Outdated
Show resolved
Hide resolved
...le.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerDbTypeTests.ValueConversions.cs
Show resolved
Hide resolved
1b66975
to
1f629be
Compare
Adds an Interval type for Spanner.