impl(bigquery): add attach_job method to query client - #6338
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an attach_job method to the BigQuery client, allowing users to bind an existing out-of-process query job reference to a high-level Query handle. The feedback focuses on the removal of the MissingProjectId error variant in favor of a generic InvalidArgument variant. The reviewer notes that this is a breaking change and violates the repository style guide regarding meaningful error distinctions. They recommend retaining MissingProjectId to preserve backward compatibility, avoid unnecessary string allocations, and keep error handling structured.
| /// An invalid argument was provided (e.g. missing job ID or project ID). | ||
| #[error("invalid argument: {0}")] | ||
| InvalidArgument(String), |
There was a problem hiding this comment.
Removing the MissingProjectId variant from the public QueryError enum is a breaking change for existing clients that match on this error. Additionally, the repository style guide states that custom error types should imply meaningful distinctions for the user and error variants should be actionable. Replacing a specific error with a generic InvalidArgument(String) makes it harder to programmatically handle a missing project ID. We should keep MissingProjectId and add InvalidArgument(String) as a new variant.
| /// An invalid argument was provided (e.g. missing job ID or project ID). | |
| #[error("invalid argument: {0}")] | |
| InvalidArgument(String), | |
| /// The project ID was not provided or could not be determined. | |
| #[error("no project ID was provided")] | |
| MissingProjectId, | |
| /// An invalid argument was provided (e.g. missing job ID). | |
| #[error("invalid argument: {0}")] | |
| InvalidArgument(String), |
References
- The repository style guide states that custom error types should imply meaningful distinctions for the user and error variants should be actionable. (link)
There was a problem hiding this comment.
I'm still on a fence on this subject and was going to bring it on the API review, but the bot brought this argument and similar to @suzmue said, having a generic InvalidArgument is not really useful and I'm leaning more towards having MissingProjectId and MissingJobId, because they are actionable in specific ways. The problem is that later if we have another required argument, are we going to keep adding MissingX ?
There was a problem hiding this comment.
That's fair.
Now that we no longer handle job_id, it's not a problem for this PR anymore, but I think we should still consider this for future cases?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6338 +/- ##
==========================================
+ Coverage 96.10% 96.11% +0.01%
==========================================
Files 282 282
Lines 73304 73443 +139
==========================================
+ Hits 70449 70591 +142
+ Misses 2855 2852 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e9deca5 to
52dde42
Compare
52dde42 to
c311945
Compare
Add
BigQuery::attach_jobto bind an existing out-of-process query job reference (JobReference) to a high-levelQueryhandle without re-submitting SQL.When #6295 was originally merged, it conflicted with #6317 (which was merged before and refactored
Querystruct fields to useQueryCreationMetadata). This caused post-merge build failures becauseattach_jobreferenced the old struct fields.This PR re-adds the changes with fixes to be compatible with
QueryCreationMetadata.