-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for range partitioning in BigQuery #2890
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This is going to need support from maintainers to show me how to make the schema beta-only, since the documentation for range-based partitioning says it is in beta. |
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs. Thanks for your contribution! A human will be with you soon. @emilymye, please review this PR or find an appropriate assignee. |
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.
A few comments:
-
I added just one comment example, but for our handwritte code, it's pretty much just ERB version checks around the beta-only code. However,
time_partioning
is also marked as experimental and isn't version checked. -
You'll need to set the range partioning values when Create/Update() and set the state value from the API in Read(). I'm going to be slightly lazy and just make you ctrl-f for
"time_partitioning "
rather than post links to each section, but they give pretty good examples of what the code should look like, especially the expand/flattenTimePartitioning helpers. As a note, we wrote this resource a while back, using the now-technically-deprecated autogenerated Golang library, but it still supports [RangePartitioning](https://godoc.org/google.golang.org/api/bigquery/v2#RangePartitioning] -
Will you be able to run tests by yourself? If not, let me know when you're ready and I can run them in a test env to verify.
@emilymye I tried setting up the environment for magic modules, but either got stuck or stopped after installing the gems. I will have to go back to see the reason for it. Will update the PR later today. |
@tristan957 let me know if you have any issues and I can try to help! |
PR has been updated besides the indentation which I left a comment on |
@emilymye I do not have a Google Cloud account to test with unfortunately. I am no longer on the project that would benefit from this PR, but I told them I would see it to completion anyway. I have also added the code for mirroring the time partitioning code. |
I also did end up getting the build environment setup. If I had an account to test with, it looks like I would be able to do that as well. |
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.
no worries! once you make the last few changes I can run the tests for you :)
third_party/terraform/website/docs/r/bigquery_table.html.markdown
Outdated
Show resolved
Hide resolved
Shouldn't these version gating flags be |
I see that all of my added code is in terraform-provider-google even though it is a beta feature |
|
That makes a lot of sense. Will fix the testing suite after work 👍 . I was probably just blind before. |
Testing has now been fixed |
@tristan957 sorry for the re-review process - I just realized a couple of things were missing. After that, it should be good and I'll start the generator again! |
@emilymye pushed fixes for latest review |
Unsure of why the diffs report that output since I have not touched those lines. |
@tristan957 the diff is a new way of generating downstream changes that we might be transitioning to in the middle of this PR! Exciting! You shouldn't need to do anything but do you mind rebasing your branch? It looks like the diff are showing you reverting some changes that other PRs made. |
@emilymye rebased 👍 |
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.
@tristan957 thank you so much for your patience! there's just a few things left but I ran the test with fixed syntax and it passed, so so once these last things are in, I'll go ahead and submit the PR! As you may have noticed, we recently started a new generation system that outputs the diff automatically in a comment, so it'll be much easier/faster for me from now on to check the diff and test your changes. It's so close!
third_party/terraform/tests/resource_bigquery_table_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_bigquery_table_test.go.erb
Outdated
Show resolved
Hide resolved
Currently only time partitioning is supported.
@emilymye no problem. I am having trouble building locally, but hopefully all your changes were addressed correctly. All that needs to happen is a bundle compile and go compile, and hopefully all will be good to go. I also went ahead and rebased into the one commit. |
third_party/terraform/tests/resource_bigquery_table_test.go.erb
Outdated
Show resolved
Hide resolved
@emilymye thanks for the approval. At what point could I see this in a beta provider release? Granted the code hasn't been merged yet. |
@tristan957 just merged, was confirming some tests! Thank you so much for contributing! I'm so sorry that it took so long to get this feature in and I missed the release cut for the release (3.10) next monday/tuesday, so it'll be in probably 3.11.0 or whatever is after 3.10 week after next. Hopefully you should have no trouble contributing in the future since you've now dealt with some of the hardest things to do! |
@emilymye thank you for your help in getting this merged. No problem with the length of time. Pretty sure 3 major holidays passed between when it opened and when it closed. Who wants to do work during the holidays? :) If I ever need another feature for the plugin, I know where to go. |
Team, I am getting following error while using the range_partitioning feature of bigquery. Any help ?resource "google_bigquery_table" "bq_tables_rgpt" { Error: Unsupported block type on bigqtable.tf line 51, in resource "google_bigquery_table" "bq_tables_rgpt": Blocks of type "range_partitioning" are not expected here. |
@sid0911 are you using the google-beta provider |
Currently only time partitioning is supported.
Release Note Template for Downstream PRs (will be copied)
hashicorp/terraform-provider-google#5239