-
Notifications
You must be signed in to change notification settings - Fork 125
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 Hive partitioning options when creating external tables #236
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. |
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
=========================================
Coverage ? 77.53%
Complexity ? 1151
=========================================
Files ? 75
Lines ? 6089
Branches ? 654
=========================================
Hits ? 4721
Misses ? 1019
Partials ? 349
Continue to review full report at Codecov.
|
*/ | ||
@SuppressWarnings("unchecked") | ||
@Nullable | ||
public abstract HivePartitioningOptions getHivePartitioningOptions(); |
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.
this breaks binary compatibility
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.
Thanks @stephaniewang526 for the review. I will update the patch and send again.
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.
@stephaniewang526, @pmakani I updated the patch and I wonder if the tests would automatically run?
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.
@stephaniewang526, thanks for triggering the tests. It was my bad with the previous patch - I missed a line that I fixed locally related to the test failures. Could you please trigger the tests again?
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.
Thanks @stephaniewang526. All tests passed now.
In general, when you make a commit please be sure to:
we don't want to do this because this renders our client unstable and would break customer's existing implementation. I suggest to try either implement without abstract and add default implementation or encapsulate with an inner method. |
@@ -135,6 +135,9 @@ public Builder setFormatOptions(FormatOptions formatOptions) { | |||
/** Sets the table schema. */ | |||
public abstract Builder setSchema(Schema schema); | |||
|
|||
/** Sets the table Hive partitioning options. */ | |||
public abstract Builder setHivePartitioningOptions(HivePartitioningOptions hivePartitioningOptions); |
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.
this breaks binary compatibility
🤖 I have created a release \*beep\* \*boop\* --- ## [1.110.0](https://www.github.com/googleapis/java-bigquery/compare/v1.109.0...v1.110.0) (2020-03-20) ### Features * add support for Hive partitioning options when creating external tables ([#235](https://www.github.com/googleapis/java-bigquery/issues/235)) ([#236](https://www.github.com/googleapis/java-bigquery/issues/236)) ([ccc2bb3](https://www.github.com/googleapis/java-bigquery/commit/ccc2bb3de28a36e3791d5441c8bdea2333877ee8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
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:
Fixes #235 ☕️