feat(bigquery): support query_results_format and Arrow in QueryRequest and QueryResponse - #16444
feat(bigquery): support query_results_format and Arrow in QueryRequest and QueryResponse#16444sachinpro wants to merge 3 commits into
Conversation
…t and QueryResponse
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 adds support for Arrow query results in the BigQuery minimal client by introducing ArrowSchema and ArrowRecordBatch structures, updating QueryRequest to support specifying the query results format, and adding serialization/deserialization logic. Feedback on the changes highlights an issue in the JSON deserialization of ArrowSchema and ArrowRecordBatch where decoding failures from UrlsafeBase64Decode are silently ignored, resulting in raw base64 strings being incorrectly assigned to the serialized fields.
| void from_json(nlohmann::json const& j, ArrowSchema& a) { | ||
| if (j.contains("serializedSchema") && j["serializedSchema"].is_string()) { | ||
| std::string b64 = j["serializedSchema"].get<std::string>(); | ||
| auto bytes = internal::UrlsafeBase64Decode(b64); | ||
| if (bytes) { | ||
| a.serialized_schema.assign( | ||
| reinterpret_cast<char const*>(bytes->data()), bytes->size()); | ||
| } else { | ||
| a.serialized_schema = b64; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If UrlsafeBase64Decode fails, falling back to assigning the raw base64 string b64 to serialized_schema is incorrect and violates the "Demand Explosive Correctness" principle. We should not silently swallow the decoding error and populate the field with invalid (undecoded) data. If decoding fails, we should avoid populating the field with the raw base64 string, as this will cause downstream parsing errors. Additionally, prefer explicit .ok() checks on the returned StatusOr object.
void from_json(nlohmann::json const& j, ArrowSchema& a) {
if (j.contains("serializedSchema") && j["serializedSchema"].is_string()) {
std::string b64 = j["serializedSchema"].get<std::string>();
auto bytes = internal::UrlsafeBase64Decode(b64);
if (bytes.ok()) {
a.serialized_schema.assign(
reinterpret_cast<char const*>(bytes->data()), bytes->size());
}
}
}References
- Demand Explosive Correctness: Never swallow errors or ignore Status types. Fail loudly and explicitly when appropriate. (link)
- Prefer defensive code, such as explicit
ok()checks, even if they seem redundant based on the current implementation of a framework, as the framework's contract may change in the future.
There was a problem hiding this comment.
Fixed in c41e2c5: Added an explicit bytes.ok() check and removed the fallback that assigned the raw base64 string to serialized_schema on decode failure.
| void from_json(nlohmann::json const& j, ArrowRecordBatch& a) { | ||
| if (j.contains("serializedRecordBatch") && | ||
| j["serializedRecordBatch"].is_string()) { | ||
| std::string b64 = j["serializedRecordBatch"].get<std::string>(); | ||
| auto bytes = internal::UrlsafeBase64Decode(b64); | ||
| if (bytes) { | ||
| a.serialized_record_batch.assign( | ||
| reinterpret_cast<char const*>(bytes->data()), bytes->size()); | ||
| } else { | ||
| a.serialized_record_batch = b64; | ||
| } | ||
| } | ||
| if (j.contains("rowCount")) { | ||
| a.row_count = GetNumberFromJson(j, "rowCount"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Similarly, if UrlsafeBase64Decode fails, falling back to assigning the raw base64 string b64 to serialized_record_batch is incorrect and violates the "Demand Explosive Correctness" principle. We should not silently swallow the decoding error and populate the field with invalid (undecoded) data. Additionally, prefer explicit .ok() checks on the returned StatusOr object.
void from_json(nlohmann::json const& j, ArrowRecordBatch& a) {
if (j.contains("serializedRecordBatch") &&
j["serializedRecordBatch"].is_string()) {
std::string b64 = j["serializedRecordBatch"].get<std::string>();
auto bytes = internal::UrlsafeBase64Decode(b64);
if (bytes.ok()) {
a.serialized_record_batch.assign(
reinterpret_cast<char const*>(bytes->data()), bytes->size());
}
}
if (j.contains("rowCount")) {
a.row_count = GetNumberFromJson(j, "rowCount");
}
}References
- Demand Explosive Correctness: Never swallow errors or ignore Status types. Fail loudly and explicitly when appropriate. (link)
- Prefer defensive code, such as explicit
ok()checks, even if they seem redundant based on the current implementation of a framework, as the framework's contract may change in the future.
There was a problem hiding this comment.
Fixed in c41e2c5: Added an explicit bytes.ok() check and removed assigning the undecoded base64 string to serialized_record_batch on decode failure.
| } | ||
| }; | ||
| std::visit(Visitor{out}, kv.second); | ||
| opentelemetry::nostd::visit(Visitor{out}, kv.second); |
|
/gcbrun |
- Revert unintended changes in opentelemetry_matchers.cc
- Fix typos linter failure on truncated string prefix in job_response_test.cc and job_query_test_utils.cc
- Apply clang-format to job_request.{h,cc} and job_response.cc
2 participants
Supports
query_results_format(ARROW_FORMAT) inQueryRequest, and parsesarrow_schema,arrow_record_batch, andpage_row_countinPostQueryResults(QueryResponse) andGetQueryResults(GetQueryResultsResponse) inbigquery_v2_minimal_internal.