-
Notifications
You must be signed in to change notification settings - Fork 118
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 partitioning and clustering in MaterializedViewDefinition #1301
feat: add support for partitioning and clustering in MaterializedViewDefinition #1301
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1301 +/- ##
============================================
- Coverage 80.18% 80.02% -0.16%
Complexity 1278 1278
============================================
Files 80 80
Lines 6665 6684 +19
Branches 771 778 +7
============================================
+ Hits 5344 5349 +5
- Misses 920 928 +8
- Partials 401 407 +6
Continue to review full report at Codecov.
|
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.
Are there other things that should get hoisted here? Reviewing https://cloud.google.com/bigquery/docs/reference/rest/v2/tables#Table and the existing StandardTableDefinition.java, I suspect we want to also pull in stats messages like numrows/numbytes/location. It doesn't appear like java ever hoisted requirePartitionFilter out of the partitioning messages, so that's something to consider as a future FR.
I think we should probs evaluate the delta and add those in in a separate PR. |
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.
Just a couple minor comments for consideration.
@@ -57,6 +57,24 @@ | |||
@Override | |||
public abstract Builder setType(Type type); | |||
|
|||
/** | |||
* Sets the time partitioning configuration for the table. If not set, the table is not |
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.
nit: do we want to update the comments here and elsewhere to indicate materialized view, rather than table?
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.
Yes -- Updated to materialized view
.
} catch (IllegalArgumentException e) { | ||
throw new IllegalArgumentException( | ||
"Illegal Argument - Got unexpected time partitioning " | ||
+ tablePb.getTimePartitioning().getType() |
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 you want to intercept like this? Seems like we may want the other fromPb() message functions to be responsible for throwing a human readable error?
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.
Okay - I removed this interception.
Source-Link: googleapis/synthtool@7956842 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:491a007c6bd6e77f9e9b1bebcd6cdf08a4a4ef2c228c123d9696845204cb685d
Towards #1300