Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

feat: add support for proto3 optional tag - #727

Merged
Linchin merged 10 commits into
googleapis:mainfrom
Dumeng:patch-1
Jan 30, 2024
Merged

feat: add support for proto3 optional tag#727
Linchin merged 10 commits into
googleapis:mainfrom
Dumeng:patch-1

Conversation

@Dumeng

Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes#726 🦕

The solution is similar to the fix in the Java client SDK.
googleapis/java-bigquerystorage#2295

@Dumeng
Dumeng requested review from a team and shollymanJanuary 2, 2024 09:17
@product-auto-labelproduct-auto-labelBot added size: xs Pull request size is extra small. api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API. labels Jan 2, 2024
@product-auto-labelproduct-auto-labelBot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Jan 17, 2024
@Linchin

Copy link
Copy Markdown
Contributor

Hi @Dumeng, thank you so much for making the contribution! Being compatible with proto3 would be very helpful.

I have a few questions/suggestions:

  1. Maybe we can add unit tests and system tests to verify that proto3 is indeed supported
  2. Should we consider adding the same treatment for read clients too?

Please let us know if you need any help, we are more than glad to work with you :)

@Dumeng

Copy link
Copy Markdown
ContributorAuthor

Hi @Linchin , Thanks for your reply.

  1. The error is coming from the server-side, so the unit test may not be able to verify the fix. I'm not familiar with the system test framework this project is using, I need some research.
  2. The ReadRowsStream seems does not need a proto description in its request. The proto description is required and causes the error in the AppendRowsStream.

1 similar comment
@Dumeng

Copy link
Copy Markdown
ContributorAuthor

Hi @Linchin , Thanks for your reply.

  1. The error is coming from the server-side, so the unit test may not be able to verify the fix. I'm not familiar with the system test framework this project is using, I need some research.
  2. The ReadRowsStream seems does not need a proto description in its request. The proto description is required and causes the error in the AppendRowsStream.

@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2024
@product-auto-labelproduct-auto-labelBot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 20, 2024
@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 24, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 24, 2024
@Linchin

Copy link
Copy Markdown
Contributor

Thanks for adding the system test! I think the system test has failed, and you can see the logs here. We also need to run the linter to pass the lint test.

Also, as you can see we only have tests for v1. Since v1beta1 is going to be deprecated soon, and we don't have tests to verify that it works, I think it's a better idea to restrain our change here to v1.

@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2024
@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 26, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 26, 2024
@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 29, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 29, 2024

@LinchinLinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding support for proto3!

@Linchin
Linchin merged commit 3b9724a into googleapis:mainJan 30, 2024
@release-pleaserelease-pleaseBot mentioned this pull request Jan 30, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigquerystorageIssues related to the googleapis/python-bigquery-storage API.size: mPull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The write stream is not compatible with proto3

3 participants

@Dumeng@Linchin@yoshi-kokoro