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 queryId to TableResult #3106

Merged
merged 6 commits into from Feb 6, 2024
Merged

Feat: Add queryId to TableResult #3106

merged 6 commits into from Feb 6, 2024

Conversation

PhongChuong
Copy link
Contributor

@PhongChuong PhongChuong commented Jan 30, 2024

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 a review from a team as a code owner 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 as a code owner January 30, 2024 17:43
@PhongChuong
Copy link
Contributor Author

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
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
Contributor Author

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.

/**
* If {@code schema} is non-null, {@code TableResult} adds the schema to {@code FieldValueList}s
* when iterating through them. {@code pageNoSchema} must not be null.
*/
@InternalApi("Exposed for testing")
public TableResult(Schema schema, long totalRows, Page<FieldValueList> pageNoSchema) {
public TableResult(
Copy link
Contributor

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
Contributor Author

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.

@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
19 checks passed
@PhongChuong PhongChuong deleted the queryId branch February 6, 2024 16:12
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: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for queryId in TableResult
5 participants