feat(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor #4681

shollyman · 2021-08-26T22:01:22Z

This functionality supports the "bring your own proto" case for writing
data.

Differences from the java equivalent:

NormalizeDescriptor return a DescriptorProto rather than ProtoSchema, to adhere to the option builder pattern (e.g. managedwriter.WithSchemaDescriptor).

Towards: #4366

This functionality supports the "bring your own proto" case for writing data. Towards: googleapis#4366

bigquery/storage/managedwriter/adapt/protoconversion.go

bigquery/storage/managedwriter/testdata/testing.pb.go

tswast

LGTM, but a few questions

tswast · 2021-08-27T14:52:11Z

bigquery/storage/managedwriter/adapt/protoconversion.go

+var normalizationSkipList = []string{
+	".google.protobuf.DoubleValue",
+	".google.protobuf.FloatValue",
+	".google.protobuf.Int64Value",


Do we support well known types now? Or is this to make sure they're supported when the backend adds support?

They're well known, so the backend shouldn't have issues resolving them without repacking. Getting them to behave as expected for the proto3 case is still the ongoing discussion. I could do some more integration testing on this front, but I wanted to get some other changes in to the main client before adding them.

If these are skipped, what is happening? Maybe it is good to have a e2e test to show the behavior.

Added integration testing, turns out the backend won't do the resolution:

integration_test.go:617: error in response: rpc error: code = InvalidArgument desc = Invalid proto schema: BqMessage.proto: testdata_WithWellKnownTypes.wrapped_int64: ".google.protobuf.Int64Value" is not defined. BqMessage.proto: testdata_WithWellKnownTypes.wrapped_string: ".google.protobuf.StringValue" is not defined. Entity: projects/shollyman-demo-test/datasets/managedwriter_test_dataset_20210827_73498187205915_0001/tables/table_20210827_73498187245922_0002/_default

Yiru, you want a bug for this?

I think what you want is a direct support on wrappers, which we currently don't have. So it should be a dup to the same bug. Since Pavan suggested a way of not needing an annotation, we could make the decision based on the BQ table schema (which means I can fix it sooner). But again, such behavior needs to be consistent across client libs.

I want direct support on wrappers, but that's tied up in the other proto3 topics. I did expect that the well known types would just work out of the box, even if the api treated them as record types with a single value field.

tswast · 2021-08-27T14:57:01Z

bigquery/storage/managedwriter/adapt/protoconversion.go

+						return nil, fmt.Errorf("error converting message %s: %v", inField.FullName(), err)
+					}
+					root.NestedType = append(root.NestedType, dp)
+					visitedTypes.delete(msgFullName)


Why do we delete this? I guess because "visited" is more about detecting recursive types? I wonder if there's a more descriptive name we could use? (Ancestor types?)

I chose the names to keep it aligned with the other implementations, but its definitely something I think we could consider. Currently the internal C++ impl is the baseline, so we'd want to start there.

Fair enough. IMO the C++ impl could have better names, then. :-)

tswast · 2021-08-27T14:58:31Z

bigquery/storage/managedwriter/adapt/protoconversion.go

+			enumFullName := string(inField.Enum().FullName())
+			enclosingTypeName := normalizeName(enumFullName) + "_E"
+			enumName := string(inField.Enum().Name())
+			actualFullName := fmt.Sprintf("%s.%s", enclosingTypeName, enumName)


Is it just enums that get the dots? Why is that? Oh, is it because we actually generated the enclosing struct so we know it's properly nested?

Yeah, gets deep into enum internals.

From the proto guide:

You can also use an enum type declared in one message as the type of a field in a different message, using the syntax MessageType.EnumType.

It's basically ensuring the enums don't collide by being explicit about the struct wrapping.

yirutang · 2021-08-27T21:44:18Z

Maybe the fix is to add the wrapper.proto as a known reference.

…

On Fri, Aug 27, 2021 at 2:40 PM shollyman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bigquery/storage/managedwriter/adapt/protoconversion.go <#4681 (comment)> : > + +func newStringSet() *stringSet { + return &stringSet{ + m: make(map[string]struct{}), + } +} + +func normalizeName(in string) string { + return strings.Replace(in, ".", "_", -1) +} + +// these types don't get normalized into the fully-contained structure. +var normalizationSkipList = []string{ + ".google.protobuf.DoubleValue", + ".google.protobuf.FloatValue", + ".google.protobuf.Int64Value", I want direct support on wrappers, but that's tied up in the other proto3 topics. I did expect that the well known types would just work out of the box, even if the api treated them as record types with a single value field. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#4681 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHGYVESHSFX54QV7GMX2TIDT7AA5FANCNFSM5C4IKWUA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Thanks. Yiru

yirutang · 2021-08-27T21:44:48Z

In that sense we can have a separate bug.

…

On Fri, Aug 27, 2021 at 2:43 PM Yiru Tang ***@***.***> wrote: Maybe the fix is to add the wrapper.proto as a known reference. On Fri, Aug 27, 2021 at 2:40 PM shollyman ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In bigquery/storage/managedwriter/adapt/protoconversion.go > <#4681 (comment)> > : > > > + > +func newStringSet() *stringSet { > + return &stringSet{ > + m: make(map[string]struct{}), > + } > +} > + > +func normalizeName(in string) string { > + return strings.Replace(in, ".", "_", -1) > +} > + > +// these types don't get normalized into the fully-contained structure. > +var normalizationSkipList = []string{ > + ".google.protobuf.DoubleValue", > + ".google.protobuf.FloatValue", > + ".google.protobuf.Int64Value", > > I want direct support on wrappers, but that's tied up in the other proto3 > topics. I did expect that the well known types would just work out of the > box, even if the api treated them as record types with a single value field. > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#4681 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AHGYVESHSFX54QV7GMX2TIDT7AA5FANCNFSM5C4IKWUA> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > -- Thanks. Yiru

-- Thanks. Yiru

shollyman added 3 commits August 26, 2021 21:56

feat(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor

410b2a6

This functionality supports the "bring your own proto" case for writing data. Towards: googleapis#4366

update comment

7bdf445

Merge branch 'master' into proto-normalizer

22c110d

shollyman requested a review from a team August 26, 2021 22:01

shollyman requested a review from a team as a code owner August 26, 2021 22:01

shollyman requested a review from tswast August 26, 2021 22:01

product-auto-label bot added the api: bigquery Issues related to the BigQuery API. label Aug 26, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 26, 2021

shollyman requested a review from yirutang August 26, 2021 22:05

shollyman added 3 commits August 26, 2021 22:07

fix comments

c991db6

goimports the new proto

b63d492

strings.ReplaceAll -> strings.Replace due to 1.11 not supporting

3ab37bc

yirutang reviewed Aug 26, 2021

View reviewed changes

bigquery/storage/managedwriter/adapt/protoconversion.go Show resolved Hide resolved

bigquery/storage/managedwriter/testdata/testing.pb.go Show resolved Hide resolved

shollyman added 2 commits August 26, 2021 23:48

add fixed64 to testing

0f35ca0

minor refactoring to align more with cl/324246878

654b05d

shollyman requested a review from codyoss August 27, 2021 00:17

goimports (again)

3c94808

tswast reviewed Aug 27, 2021

View reviewed changes

shollyman requested review from yirutang and tswast August 27, 2021 16:33

tswast approved these changes Aug 27, 2021

View reviewed changes

shollyman added 5 commits August 27, 2021 20:27

augment testing: integration, well known types

3f376fc

update conversion test for well known types

c3ac1e0

commenting and formatting pass

5b8711f

Merge branch 'master' into proto-normalizer

567026d

fix variable naming

9bc1b01

codyoss approved these changes Aug 27, 2021

View reviewed changes

Merge branch 'master' into proto-normalizer

2b5f439

shollyman added the automerge Merge the pull request once unit tests and other checks pass. label Aug 27, 2021

shollyman merged commit c54aa74 into googleapis:master Aug 27, 2021

shollyman deleted the proto-normalizer branch August 27, 2021 22:50

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor #4681

feat(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor #4681

shollyman commented Aug 26, 2021 •

edited

Loading

tswast left a comment

tswast Aug 27, 2021

shollyman Aug 27, 2021

yirutang Aug 27, 2021

shollyman Aug 27, 2021

yirutang Aug 27, 2021

shollyman Aug 27, 2021

tswast Aug 27, 2021

shollyman Aug 27, 2021

tswast Aug 27, 2021

tswast Aug 27, 2021

shollyman Aug 27, 2021

yirutang commented Aug 27, 2021 via email

yirutang commented Aug 27, 2021 via email

feat(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor #4681

feat(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor #4681

Conversation

shollyman commented Aug 26, 2021 • edited Loading

tswast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yirutang commented Aug 27, 2021 via email

yirutang commented Aug 27, 2021 via email

shollyman commented Aug 26, 2021 •

edited

Loading