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 manual wrapper for v1beta2 read client #117

merged 7 commits into from Jan 22, 2021


Copy link

@tswast tswast commented Jan 14, 2021

We got a bit lucky in that the reader module doesn't create any proto objects, so it could be reused without modification from the "v1" endpoint. Maybe we should be defining these in a shared module that doesn't include the version information, though?

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:

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@tswast tswast requested a review from as a code owner Jan 14, 2021
@tswast tswast requested review from stephaniewang526 and removed request for Jan 14, 2021
@product-auto-label product-auto-label bot added the api: bigquerystorage label Jan 14, 2021
@google-cla google-cla bot added the cla: yes label Jan 14, 2021
@tswast tswast requested review from shollyman and steffnay and removed request for stephaniewang526 Jan 14, 2021
Copy link
Contributor Author

@tswast tswast commented Jan 15, 2021

Kokoro failure:

    def test_snapshot(client, project_id, table_with_data_ref, bq_client):
>       before_new_data = types.Timestamp()
E       AttributeError: module '' has no attribute 'Timestamp'

I think there's a manual config for adding these types from the core protobuf. Will need to add before merging this PR.

Copy link
Contributor Author

@tswast tswast commented Jan 15, 2021

Re: AttributeError: module '' has no attribute 'Timestamp', the v1 API has a handwritten module which adds these.

@plamut Was this just for backwards compatibility? I imagine I could use the protobuf/proto-plus well-known type logic here instead.

Copy link

@plamut plamut commented Jan 16, 2021

@tswast Since I was not that familiar with this library and short on time, I think I included these generated types like that to avoid import errors and unblock the microgenerator transition. I didn't explore if there exist dedicated synth settings for the proto-plus types, but instead simply adjusted the existing logic for injecting the (old) protos.

Copy link
Contributor Author

@tswast tswast commented Jan 20, 2021

This is ready for review. I used some parameterized test fixtures to keep from duplicating code in the system tests. Next, I'll start looking more closely at what needs to be done for the write client.

@tswast tswast requested a review from plamut Jan 22, 2021
@tswast tswast merged commit 798cd34 into googleapis:master Jan 22, 2021
9 checks passed
@tswast tswast deleted the v1beta2-reader branch Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
api: bigquerystorage cla: yes
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants