-
Notifications
You must be signed in to change notification settings - Fork 6
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: implement support for more well-known-types #10
Conversation
26018fb
to
f3b8b34
Compare
- google.protobuf.Struct (mapped to a JSON STRING) - google.type.TimeOfDay (mapped to TIME) - google.type.Date (mapped to DATE) - google.type.LatLng (mapped to GEOGRAPHY) The reason for using GEOGRAPHY as default for LatLng is because of the clustering opportunities for tables that store locations as GEOGRAPHY. Future SchemaOptions may override to store LatLng as a record instead. Integration tested against public BigQuery datasets. The intention is to identify datasets that exercise the features of this library, then perform a load/marshal/unmarshal sequence and validate that the original result is preserved.
Why not map proto struct to a BigQuery struct as well (I think they call it a record in Go)? |
@alethenorio google.protobuf.Struct is an untyped struct without a schema - can't be mapped 1:1 with a BigQuery RECORD. |
Ah I see, it's just a map. 👍 |
@@ -17,65 +17,111 @@ import ( | |||
) | |||
|
|||
func Test_Integration_PublicDataSets(t *testing.T) { | |||
t.Parallel() |
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 generally take it as a convention to not use parallelization in tests. It makes things more complicated than they need to be.
See https://speakerdeck.com/mitchellh/advanced-testing-with-go?slide=67
Do we really need it here?
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.
Each instance of the test takes ~5s to run depending on connection time to BigQuery - some datasets seem to be rate limited.
The latest version of GolangCI-Lint comes with a linter for t.Parallel()
. Do you think we should disable it? I think it's good that it makes tests run faster, and that it enforces to not share resources between tests.
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.
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.
From the docs I understand the linter will enforce bad usage of t.Parallel so I see no reason to disable it.
I think we can make educated uses of t.Parallel and this sounds like one of them given the nature of the integration tests, just need to be careful, specially in integration tests as sharing of resources (such as a database) is more prone to happen.
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.
Sounds good to me!
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.
Very nice work. Keep it up
@alethenorio Thanks! 🙌 |
The reason for using GEOGRAPHY as default for LatLng is because of the
clustering opportunities for tables that store locations as GEOGRAPHY.
Future SchemaOptions may override to store LatLng as a record instead.
Integration tested against public BigQuery datasets. The intention is to
identify datasets that exercise the features of this library, then
perform a load/marshal/unmarshal sequence and validate that the original
result is preserved.