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(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor #4681
feat(bigquery/storage/managedwriter/adapt): add NormalizeDescriptor #4681
Changes from 9 commits
410b2a6
7bdf445
22c110d
c991db6
b63d492
3ab37bc
0f35ca0
654b05d
3c94808
3f376fc
c3ac1e0
5b8711f
567026d
9bc1b01
2b5f439
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.
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?)
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 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.
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.
Fair enough. IMO the C++ impl could have better names, then. :-)
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 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?
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.
Yeah, gets deep into enum internals.
From the proto guide:
It's basically ensuring the enums don't collide by being explicit about the struct wrapping.
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.
Do we support well known types now? Or is this to make sure they're supported when the backend adds support?
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.
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.
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.
If these are skipped, what is happening? Maybe it is good to have a e2e test to show the behavior.
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.
Added integration testing, turns out the backend won't do the resolution:
Yiru, you want a bug for this?
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 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.
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 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.