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 missing storage related fields to Table, TableInfo and StandardTableDefinition #2673

Merged

Conversation

sumeetgajjar
Copy link
Contributor

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #2672 ☕️

@sumeetgajjar sumeetgajjar requested a review from a team as a code owner April 29, 2023 00:51
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: bigquery Issues related to the googleapis/java-bigquery API. labels Apr 29, 2023
@sumeetgajjar
Copy link
Contributor Author

@Neenu1995 @farhan0102 can you please review this PR?

Thanks in advance

@@ -1451,6 +1451,9 @@ public void testCreateAndGetTable() {
assertNotNull(remoteTable.getLastModifiedTime());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumBytes());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumLongTermBytes());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumTotalLogicalBytes());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumActiveLogicalBytes());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumLongTermLogicalBytes());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: We cannot add the rest of the newly added fields like TimeTravelBytes etc since they are not available for a table created few seconds ago.

@Neenu1995 Neenu1995 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels May 1, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2023
@sumeetgajjar sumeetgajjar changed the title Add missing storage related fields to Table, TableInfo and StandardTableDefinition chore: Add missing storage related fields to Table, TableInfo and StandardTableDefinition May 1, 2023
@Neenu1995 Neenu1995 added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 2, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 2, 2023
@sumeetgajjar
Copy link
Contributor Author

The ITBigQueryTest.testCreateAndGetTable integration test is failing

[ERROR] com.google.cloud.bigquery.it.ITBigQueryTest.testCreateAndGetTable  Time elapsed: 0.524 s  <<< FAILURE!
java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertNotNull(Assert.java:713)
	at org.junit.Assert.assertNotNull(Assert.java:723)
	at com.google.cloud.bigquery.it.ITBigQueryTest.testCreateAndGetTable(ITBigQueryTest.java:1454)

This is due to the fact there's a bug in the generated code of google-api-services-bigquery

https://github.com/googleapis/google-api-java-client-services/blob/09b3ea51aac423a1529b459d82abd5db736d4ed9/clients/google-api-services-bigquery/v2/2.0.0/com/google/api/services/bigquery/model/Table.java#L260-L261

The JSON mapper is expecting the key to be num_total_logical_bytes (snake cased).
However, the actual JSON object has the key as numTotalLogicalBytes (camel cased).

I ran the integration test while attached to a debugger and inspected the com.google.api.services.bigquery.model.Table object which is feed into TableInfo.BuilderImpl constructor.

tablePb.getNumTotalLogicalBytes() // shall return null
tablePb.get("numTotalLogicalBytes") // shall return a valid long value

I have filed an issue googleapis/google-api-java-client-services#16915

@Neenu1995 @prash-mi I took a look at the google-api-services-bigquery but it seems there input JSON required for generating the bigquery client is not open-sourced or at least not available in that repository, please correct me if I am wrong and please let me know how to proceed next?

@Neenu1995 Neenu1995 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 3, 2023
@sumeetgajjar
Copy link
Contributor Author

Hi @Neenu1995 - based on this comment googleapis/google-api-java-client-services#16915 (comment) from shollyman, I checked the discovery document and the revision number is 20230520 which is beyond 20230504.

I ran a test of my own to verify the newly added fields are populated.

Can you please re-run the necessary tests and remove the do not merge label from this PR?

@Neenu1995 Neenu1995 added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. and removed automerge Merge the pull request once unit tests and other checks pass. labels Jun 21, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 21, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 21, 2023
@sumeetgajjar
Copy link
Contributor Author

Hi @Neenu1995 thanks for adding run labels.

It seems the ci / clirr job failed since we added a bunch of new methods.
Can you please share steps/actions required from my-side to mitigate this error?

Error:  Unable to locate enclosing class com.google.cloud.bigquery. for nested class com.google.cloud.bigquery.$AutoValue_Labels
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition: Abstract method 'public java.lang.Long getNumActiveLogicalBytes()' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition: Abstract method 'public java.lang.Long getNumActivePhysicalBytes()' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition: Abstract method 'public java.lang.Long getNumLongTermLogicalBytes()' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition: Abstract method 'public java.lang.Long getNumLongTermPhysicalBytes()' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition: Abstract method 'public java.lang.Long getNumTimeTravelPhysicalBytes()' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition: Abstract method 'public java.lang.Long getNumTotalLogicalBytes()' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition: Abstract method 'public java.lang.Long getNumTotalPhysicalBytes()' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition$Builder: Abstract method 'public com.google.cloud.bigquery.StandardTableDefinition$Builder setNumActiveLogicalBytes(java.lang.Long)' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition$Builder: Abstract method 'public com.google.cloud.bigquery.StandardTableDefinition$Builder setNumActivePhysicalBytes(java.lang.Long)' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition$Builder: Abstract method 'public com.google.cloud.bigquery.StandardTableDefinition$Builder setNumLongTermLogicalBytes(java.lang.Long)' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition$Builder: Abstract method 'public com.google.cloud.bigquery.StandardTableDefinition$Builder setNumLongTermPhysicalBytes(java.lang.Long)' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition$Builder: Abstract method 'public com.google.cloud.bigquery.StandardTableDefinition$Builder setNumTimeTravelPhysicalBytes(java.lang.Long)' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition$Builder: Abstract method 'public com.google.cloud.bigquery.StandardTableDefinition$Builder setNumTotalLogicalBytes(java.lang.Long)' has been added
Error:  7013: com.google.cloud.bigquery.StandardTableDefinition$Builder: Abstract method 'public com.google.cloud.bigquery.StandardTableDefinition$Builder setNumTotalPhysicalBytes(java.lang.Long)' has been added
[INFO] ------------------------------------------------------------------------

@Neenu1995
Copy link
Contributor

To fix, the clirr check, please add exceptions to the clirr-ignored-differences file according to this doc.

@sumeetgajjar
Copy link
Contributor Author

To fix, the clirr check, please add exceptions to the clirr-ignored-differences file according to this doc.

Thanks @Neenu1995 for the doc, fixed it in the latest commit.
Please re-run the checks 😄

@Neenu1995 Neenu1995 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jun 23, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 23, 2023
@Neenu1995 Neenu1995 changed the title chore: Add missing storage related fields to Table, TableInfo and StandardTableDefinition feat: Add missing storage related fields to Table, TableInfo and StandardTableDefinition Jun 23, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2023
@@ -2863,4 +2865,27 @@ public void testTestIamPermissionsWhenNoPermissionsGranted() {
assertEquals(perms, ImmutableList.of());
verify(bigqueryRpcMock).testIamPermissions(resourceId, checkedPermissions, EMPTY_RPC_OPTIONS);
}

@Test
public void testSumeet() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clean up personal tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the personal test(s).
Thanks for pointing it out.

Please re-run the checks 😄

@Neenu1995 Neenu1995 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jun 23, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 23, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2023
@Neenu1995 Neenu1995 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2023
@Neenu1995 Neenu1995 self-requested a review June 23, 2023 17:22
@Neenu1995 Neenu1995 added automerge Merge the pull request once unit tests and other checks pass. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jun 23, 2023
@gcf-merge-on-green gcf-merge-on-green bot merged commit e3003f4 into googleapis:main Jun 23, 2023
16 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 23, 2023
@sumeetgajjar
Copy link
Contributor Author

Thank you @Neenu1995 for approving the PR and guiding us with TODOs to get in this change.

Looking forward to future contributions.

@Neenu1995
Copy link
Contributor

Thanks for contributing ti the Bigquery library @sumeetgajjar

gcf-merge-on-green bot pushed a commit that referenced this pull request Jul 17, 2023
🤖 I have created a release *beep* *boop*
---


## [2.30.0](https://togithub.com/googleapis/java-bigquery/compare/v2.29.0...v2.30.0) (2023-07-17)


### Features

* Add missing storage related fields to Table, TableInfo and StandardTableDefinition ([#2673](https://togithub.com/googleapis/java-bigquery/issues/2673)) ([e3003f4](https://togithub.com/googleapis/java-bigquery/commit/e3003f48df9cca2bd549d893ffef3bb198a3b2aa))
* Add support for Search statistics ([#2787](https://togithub.com/googleapis/java-bigquery/issues/2787)) ([344f695](https://togithub.com/googleapis/java-bigquery/commit/344f695e319470acf350ebdd56d643c03704ea1f))


### Dependencies

* Update dependency com.google.api.grpc:proto-google-cloud-bigqueryconnection-v1 to v2.22.0 ([#2777](https://togithub.com/googleapis/java-bigquery/issues/2777)) ([078f244](https://togithub.com/googleapis/java-bigquery/commit/078f244572db7484471d2c55a0db4533de0d1dc7))
* Update dependency com.google.cloud:google-cloud-datacatalog-bom to v1.26.0 ([#2778](https://togithub.com/googleapis/java-bigquery/issues/2778)) ([2ee52c9](https://togithub.com/googleapis/java-bigquery/commit/2ee52c934d253d29c16b25d498ebe8e968cda481))
* Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.13.0 ([#2786](https://togithub.com/googleapis/java-bigquery/issues/2786)) ([dd14eee](https://togithub.com/googleapis/java-bigquery/commit/dd14eee126f3cb6be7c943157e65acd5d4a088d4))
* Update github/codeql-action action to v2.20.1 ([#2766](https://togithub.com/googleapis/java-bigquery/issues/2766)) ([2014613](https://togithub.com/googleapis/java-bigquery/commit/201461351ac9813f6d11e6f5c3b9ec4dd01c001b))
* Update github/codeql-action action to v2.20.4 ([#2784](https://togithub.com/googleapis/java-bigquery/issues/2784)) ([e886f5f](https://togithub.com/googleapis/java-bigquery/commit/e886f5fa79aee469fe7b8860b5e87951635b6ce7))
* Update ossf/scorecard-action action to v2.2.0 ([#2775](https://togithub.com/googleapis/java-bigquery/issues/2775)) ([688b2a0](https://togithub.com/googleapis/java-bigquery/commit/688b2a0b16b578dc0784094608b35cb3a68f151b))

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
@sumeetgajjar sumeetgajjar deleted the add_storage_fields_to_table branch February 2, 2024 04:10
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. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature parity for storage related fields across REST API and Java SDK
3 participants