Skip to content

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
@sumeetgajjar sumeetgajjar requested a review from prash-mi 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 sumeetgajjar force-pushed the add_storage_fields_to_table branch from c3c117b to df85341 Compare April 29, 2023 00:53
@sumeetgajjar
Copy link
Contributor Author

@Neenu1995 @farhan0102 can you please review this PR?

Thanks in advance

assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumLongTermBytes());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumTotalLogicalBytes());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumActiveLogicalBytes());
assertNotNull(remoteTable.<StandardTableDefinition>getDefinition().getNumLongTermLogicalBytes());
Copy link
Contributor Author

@sumeetgajjar sumeetgajjar Apr 29, 2023

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 sumeetgajjar force-pushed the add_storage_fields_to_table branch from 965557e to a39b96a Compare June 21, 2023 22:03
@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
}

@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
@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