impl(bigquery): add query runner builder#5870
Conversation
product-auto-label
Bot
added
the
api: bigquery
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces custom error handling and query execution scaffolding for the Cloud BigQuery query client, including a new QueryError enum and a unified RunQuery request builder. The review feedback highlights opportunities to optimize performance and simplify the design by removing unnecessary String clones when values are consumed, and eliminating a redundant Arc wrapper around the underlying RPC error since QueryError does not implement Clone.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5870 +/- ##
=======================================
Coverage 97.88% 97.88%
=======================================
Files 230 231 +1
Lines 58104 58192 +88
=======================================
+ Hits 56875 56964 +89
+ Misses 1229 1228 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alvarowolfx
changed the title
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
I think the new() should be pub(crate), but nothing else is blocking.
| request: RunQueryRequest::default() | ||
| .set_query(sql) | ||
| .set_use_legacy_sql(wkt::BoolValue::from(false)), |
There was a problem hiding this comment.
non-blocking Q: default to job creation mode optional?
|
|
||
| impl RunQuery { | ||
| /// Creates a new `RunQuery` builder for the given SQL query. | ||
| pub fn new(job_service: Arc<JobService>, sql: String) -> Self { |
| include!("../generated/run_query_builder.rs"); | ||
| include!("../generated/run_query_request.rs"); |
There was a problem hiding this comment.
style comment: we should prefer to have a mod generated over include!(), e.g.: https://github.com/googleapis/google-cloud-rust/blob/main/src/storage/src/generated.rs
The compiler can do smarter things if we build the module tree vs. pasting code in. (I think, I could understand the why better).
I think you would have to mess around with the modules of the generated types though, so feel free to take a TODO and not block this PR.
| let mut run_query = RunQuery::new(job_service, "SELECT 1".to_string()); | ||
| assert!(!run_query.request.force_job_path()); | ||
|
|
||
| // Set allow_large_results |
There was a problem hiding this comment.
remove? comment is not adding to the code.
| /// Executes the SQL query, routing internally to `jobs.query` (fast path) | ||
| /// or `jobs.insert` (job path) depending on configured fields. |
There was a problem hiding this comment.
nit:
| /// Executes the SQL query, routing internally to `jobs.query` (fast path) | |
| /// or `jobs.insert` (job path) depending on configured fields. | |
| /// Executes the SQL query | |
| /// | |
| /// The implementation routes internally to `jobs.query` (fast path) | |
| /// or `jobs.insert` (job path) depending on configured fields. If the fast path is available, the client library takes it. If not, it falls back to creating a job, which is typically slower. |
^ probably need to reformat. And links are helpful.
2 participants
Towards #5844