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 partitioning and clustering in MaterializedViewDefinition #1301

Merged

Conversation

stephaniewang526
Copy link
Contributor

Towards #1300

@stephaniewang526 stephaniewang526 requested a review from a team May 11, 2021 18:04
@stephaniewang526 stephaniewang526 requested a review from a team as a code owner May 11, 2021 18:04
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 11, 2021
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/java-bigquery API. label May 11, 2021
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #1301 (08b7dbf) into master (b6ebd14) will decrease coverage by 0.15%.
The diff coverage is 26.31%.

❗ Current head 08b7dbf differs from pull request most recent head 3d92915. Consider uploading reports for the commit 3d92915 to get more accurate results
Impacted file tree graph

@@             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     
Impacted Files Coverage Δ Complexity Δ
...gle/cloud/bigquery/MaterializedViewDefinition.java 54.00% <26.31%> (-16.97%) 6.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6ebd14...3d92915. Read the comment docs.

Copy link
Contributor

@shollyman shollyman left a 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.

@stephaniewang526
Copy link
Contributor Author

stephaniewang526 commented May 13, 2021

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.
Created #1310 to track this work.

Copy link
Contributor

@shollyman shollyman left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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()
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@stephaniewang526 stephaniewang526 added the automerge Merge the pull request once unit tests and other checks pass. label May 14, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit b909754 into googleapis:master May 14, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 14, 2021
gcf-owl-bot bot added a commit that referenced this pull request Jan 4, 2022
Source-Link: googleapis/synthtool@7956842
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:491a007c6bd6e77f9e9b1bebcd6cdf08a4a4ef2c228c123d9696845204cb685d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/java-bigquery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants