Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

Feat: Add queryId to TableResult#3106

Merged
PhongChuong merged 6 commits intomainfrom
queryId
Feb 6, 2024
Merged

Feat: Add queryId to TableResult#3106
PhongChuong merged 6 commits intomainfrom
queryId

Conversation

@PhongChuong
Copy link
Copy Markdown
Contributor

@PhongChuong PhongChuong commented Jan 30, 2024 •
edited
Loading

This PR adds queryId to TableResult by changing its public constructors. It should be fine to update TableResult constructors to include QueryId since it is used to return results to the user and users should not directly construct TableResult objects.

  • 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 #3105 ☕️

@PhongChuong PhongChuong requested review from a team and tswast January 30, 2024 17:40
@product-auto-label product-auto-label Bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/java-bigquery API. labels Jan 30, 2024
@PhongChuong PhongChuong requested a review from a team January 30, 2024 17:43
@PhongChuong
Copy link
Copy Markdown
Contributor Author

PhongChuong commented Jan 30, 2024

Please let me know if it's not advisable to update the constructor for TableResult.

@tswast tswast requested review from shollyman and removed request for tswast January 30, 2024 18:47
@tswast
Copy link
Copy Markdown
Contributor

tswast commented Jan 30, 2024

Please let me know if it's not advisable to update the constructor for TableResult.

I recommend asking on a Java chat. This may be considered a breaking change.

@PhongChuong PhongChuong added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jan 30, 2024
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jan 30, 2024
@PhongChuong
Copy link
Copy Markdown
Contributor Author

PhongChuong commented Jan 30, 2024

Please let me know if it's not advisable to update the constructor for TableResult.

I recommend asking on a Java chat. This may be considered a breaking change.

Just verified, since the constructors are annotated with
@internalapi("Exposed for testing")
It should be fine to change the constructor.

Neenu1995
Neenu1995 reviewed Jan 31, 2024
View reviewed changes
Comment thread google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/it/ITBigQueryTest.java
Comment thread google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/it/ITBigQueryTest.java
shollyman
shollyman reviewed Jan 31, 2024
View reviewed changes
Comment thread google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/TableResult.java
*/
@InternalApi("Exposed for testing")
public TableResult(Schema schema, long totalRows, Page<FieldValueList> pageNoSchema) {
public TableResult(
Copy link
Copy Markdown
Contributor

@shollyman shollyman Jan 31, 2024

Choose a reason for hiding this comment

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

Rather than solely extending the existing constructor, should we just provide an second signature or a builder? It would mean you can avoid having to touch all the callsites where queryId isn't relevant for construction.

Copy link
Copy Markdown
Contributor Author

@PhongChuong PhongChuong Feb 1, 2024

Choose a reason for hiding this comment

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

As discussed offline. I think creating a builder would be the preferred option. I'll follow up with another PR to switch to using a builder.

Comment thread google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/it/ITBigQueryTest.java
@PhongChuong PhongChuong added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Feb 1, 2024
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 1, 2024
@PhongChuong PhongChuong merged commit 2156f02 into main Feb 6, 2024
@PhongChuong PhongChuong deleted the queryId branch February 6, 2024 16:12
@release-please release-please Bot mentioned this pull request Feb 6, 2024
Merged
This was referenced Feb 13, 2024
Closed
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@Neenu1995 Neenu1995 Neenu1995 left review comments

@shollyman shollyman shollyman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigquery Issues related to the googleapis/java-bigquery API. size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for queryId in TableResult

5 participants

@PhongChuong @tswast @shollyman @Neenu1995 @yoshi-kokoro