Skip to content
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: add support for proto3 optional tag #727

Merged
merged 10 commits into from
Jan 30, 2024
Merged

Conversation

Dumeng
Copy link
Contributor

@Dumeng Dumeng commented Jan 2, 2024

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 as code owners January 2, 2024 09:17
@Dumeng Dumeng requested a review from shollyman January 2, 2024 09:17
@product-auto-label product-auto-label bot 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-label product-auto-label bot 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
Contributor

Linchin commented Jan 17, 2024

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
Contributor Author

Dumeng commented Jan 18, 2024

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
Contributor Author

Dumeng commented Jan 18, 2024

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.

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

Linchin commented Jan 24, 2024

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.

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

@Linchin Linchin left a comment

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:main Jan 30, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API. size: m Pull 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