feat: implement support for more well-known-types #10
Conversation
odsod
requested review from
alethenorio,
alfredgunnar and
Jassob
as code owners
odsod
force-pushed
the
wkt2
branch
4 times, most recently
from
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.
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.
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.
There was a problem hiding this comment.
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.
|
@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.