Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/job_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ void to_json(nlohmann::json& j, QueryRequest const& q) {
{"formatOptions", q.format_options()},
{"labels", q.labels()}};

if (!q.query_results_format().empty()) {
j["queryResultsFormat"] = q.query_results_format();
}

ToIntJson(q.timeout(), j,
"timeoutMs"); // timeoutMs value is a number for this request type.
}
Expand Down Expand Up @@ -309,6 +313,8 @@ void from_json(nlohmann::json const& j, QueryRequest& q) {
SafeGetTo(j, "defaultDataset", &QueryRequest::set_default_dataset, q);
SafeGetTo(j, "formatOptions", &QueryRequest::set_format_options, q);
SafeGetTo(j, "labels", &QueryRequest::set_labels, q);
SafeGetTo(j, "queryResultsFormat", &QueryRequest::set_query_results_format,
q);

std::chrono::milliseconds timeout;
FromJson(timeout, j, "timeoutMs");
Expand Down Expand Up @@ -370,6 +376,7 @@ std::string QueryRequest::DebugString(absl::string_view name,
.SubMessage("default_dataset", default_dataset())
.SubMessage("format_options", format_options())
.SubMessage("job_creation_mode", job_creation_mode())
.StringField("query_results_format", query_results_format())
.Build();
}

Expand Down
12 changes: 12 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/job_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,17 @@ class QueryRequest {
return std::move(set_job_creation_mode(std::move(job_creation_mode)));
}

std::string const& query_results_format() const {
return query_results_format_;
}
QueryRequest& set_query_results_format(std::string query_results_format) & {
query_results_format_ = std::move(query_results_format);
return *this;
}
QueryRequest&& set_query_results_format(std::string query_results_format) && {
return std::move(set_query_results_format(std::move(query_results_format)));
}

std::string DebugString(absl::string_view name,
TracingOptions const& options = {},
int indent = 0) const;
Expand All @@ -548,6 +559,7 @@ class QueryRequest {
std::string parameter_mode_;
std::string location_;
std::string request_id_;
std::string query_results_format_;

bool dry_run_ = false;
bool preserve_nulls_ = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1029,7 +1029,8 @@ TEST(PostQueryRequestTest, DebugString) {
R"( labels { key: "lk1" value: "lv1" } labels { key: "lk2" value: "lv2" })"
R"( default_dataset { project_id: "2" dataset_id: "1" })"
R"( format_options { use_int64_timestamp: true timestamp_output_format: TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED })"
R"( job_creation_mode { value: "JOB_CREATION_MODE_UNSPECIFIED" } } })");
R"( job_creation_mode { value: "JOB_CREATION_MODE_UNSPECIFIED" })"
R"( query_results_format: "ARROW" } })");

EXPECT_EQ(
request.DebugString(
Expand All @@ -1050,7 +1051,8 @@ TEST(PostQueryRequestTest, DebugString) {
R"( labels { key: "lk1" value: "lv1" } labels { key: "lk2" value: "lv2" })"
R"( default_dataset { project_id: "2" dataset_id: "1" })"
R"( format_options { use_int64_timestamp: true timestamp_output_format: TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED })"
R"( job_creation_mode { value: "JOB_CRE...<truncated>..." } } })");
R"( job_creation_mode { value: "JOB_CRE...<truncated>..." })"
R"( query_results_format: "ARROW" } })");

EXPECT_EQ(request.DebugString("PostQueryRequest", TracingOptions{}.SetOptions(
"single_line_mode=F")),
Expand Down Expand Up @@ -1108,6 +1110,7 @@ TEST(PostQueryRequestTest, DebugString) {
job_creation_mode {
value: "JOB_CREATION_MODE_UNSPECIFIED"
}
query_results_format: "ARROW"
}
})");
}
Expand Down
100 changes: 100 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/job_response.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include "google/cloud/bigquery/v2/minimal/internal/job_response.h"
#include "google/cloud/bigquery/v2/minimal/internal/json_utils.h"
#include "google/cloud/internal/base64_transforms.h"
#include "google/cloud/internal/debug_string.h"
#include "google/cloud/internal/make_status.h"
#include "absl/strings/str_cat.h"
Expand Down Expand Up @@ -192,6 +193,60 @@ std::string CancelJobResponse::DebugString(absl::string_view name,
.Build();
}

std::string ArrowSchema::DebugString(absl::string_view name,
TracingOptions const& options,
int indent) const {
return internal::DebugFormatter(name, options, indent)
.StringField("serialized_schema", serialized_schema)
.Build();
}

std::string ArrowRecordBatch::DebugString(absl::string_view name,
TracingOptions const& options,
int indent) const {
return internal::DebugFormatter(name, options, indent)
.StringField("serialized_record_batch", serialized_record_batch)
.Field("row_count", row_count)
.Build();
}

void to_json(nlohmann::json& j, ArrowSchema const& a) {
j = nlohmann::json{
{"serializedSchema", internal::UrlsafeBase64Encode(a.serialized_schema)}};
}

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());
}
}
}
Comment on lines +218 to +227

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. Demand Explosive Correctness: Never swallow errors or ignore Status types. Fail loudly and explicitly when appropriate. (link)
  2. 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 to_json(nlohmann::json& j, ArrowRecordBatch const& a) {
j = nlohmann::json{{"serializedRecordBatch",
internal::UrlsafeBase64Encode(a.serialized_record_batch)},
{"rowCount", std::to_string(a.row_count)}};
}

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");
}
}
Comment on lines +235 to +248

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. Demand Explosive Correctness: Never swallow errors or ignore Status types. Fail loudly and explicitly when appropriate. (link)
  2. 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c41e2c5: Added an explicit bytes.ok() check and removed assigning the undecoded base64 string to serialized_record_batch on decode failure.


std::string PostQueryResults::DebugString(absl::string_view name,
TracingOptions const& options,
int indent) const {
Expand All @@ -209,6 +264,9 @@ std::string PostQueryResults::DebugString(absl::string_view name,
.SubMessage("job_reference", job_reference)
.SubMessage("session_info", session_info)
.SubMessage("dml_stats", dml_stats)
.SubMessage("arrow_schema", arrow_schema)
.SubMessage("arrow_record_batch", arrow_record_batch)
.Field("page_row_count", page_row_count)
.Build();
}

Expand Down Expand Up @@ -262,6 +320,11 @@ StatusOr<QueryResponse> QueryResponse::BuildFromHttpResponse(

SafeGetTo(query_results.session_info, *json, "sessionInfo");
SafeGetTo(query_results.dml_stats, *json, "dmlStats");
SafeGetTo(query_results.arrow_schema, *json, "arrowSchema");
SafeGetTo(query_results.arrow_record_batch, *json, "arrowRecordBatch");
if (json->contains("pageRowCount")) {
query_results.page_row_count = GetNumberFromJson(*json, "pageRowCount");
}

QueryResponse response;
response.http_response = http_response;
Expand Down Expand Up @@ -299,6 +362,15 @@ void to_json(nlohmann::json& j, PostQueryResults const& q) {
{"errors", q.errors},
{"sessionInfo", q.session_info},
{"dmlStats", q.dml_stats}};
if (!q.arrow_schema.serialized_schema.empty()) {
j["arrowSchema"] = q.arrow_schema;
}
if (!q.arrow_record_batch.serialized_record_batch.empty()) {
j["arrowRecordBatch"] = q.arrow_record_batch;
}
if (q.page_row_count > 0) {
j["pageRowCount"] = std::to_string(q.page_row_count);
}
}

void from_json(nlohmann::json const& j, PostQueryResults& q) {
Expand All @@ -315,6 +387,11 @@ void from_json(nlohmann::json const& j, PostQueryResults& q) {
SafeGetTo(q.errors, j, "errors");
SafeGetTo(q.session_info, j, "sessionInfo");
SafeGetTo(q.dml_stats, j, "dmlStats");
SafeGetTo(q.arrow_schema, j, "arrowSchema");
SafeGetTo(q.arrow_record_batch, j, "arrowRecordBatch");
if (j.contains("pageRowCount")) {
q.page_row_count = GetNumberFromJson(j, "pageRowCount");
}
}

void to_json(nlohmann::json& j, GetQueryResults const& q) {
Expand All @@ -330,6 +407,15 @@ void to_json(nlohmann::json& j, GetQueryResults const& q) {
{"jobReference", q.job_reference},
{"rows", q.rows},
{"errors", q.errors}};
if (!q.arrow_schema.serialized_schema.empty()) {
j["arrowSchema"] = q.arrow_schema;
}
if (!q.arrow_record_batch.serialized_record_batch.empty()) {
j["arrowRecordBatch"] = q.arrow_record_batch;
}
if (q.page_row_count > 0) {
j["pageRowCount"] = std::to_string(q.page_row_count);
}
}
void from_json(nlohmann::json const& j, GetQueryResults& q) {
SafeGetTo(q.kind, j, "kind");
Expand All @@ -344,6 +430,11 @@ void from_json(nlohmann::json const& j, GetQueryResults& q) {
SafeGetTo(q.job_reference, j, "jobReference");
SafeGetTo(q.rows, j, "rows");
SafeGetTo(q.errors, j, "errors");
SafeGetTo(q.arrow_schema, j, "arrowSchema");
SafeGetTo(q.arrow_record_batch, j, "arrowRecordBatch");
if (j.contains("pageRowCount")) {
q.page_row_count = GetNumberFromJson(j, "pageRowCount");
}
}

std::string GetQueryResults::DebugString(absl::string_view name,
Expand All @@ -362,6 +453,9 @@ std::string GetQueryResults::DebugString(absl::string_view name,
.Field("errors", errors)
.SubMessage("schema", schema)
.SubMessage("job_reference", job_reference)
.SubMessage("arrow_schema", arrow_schema)
.SubMessage("arrow_record_batch", arrow_record_batch)
.Field("page_row_count", page_row_count)
.Build();
}

Expand Down Expand Up @@ -413,6 +507,12 @@ GetQueryResultsResponse::BuildFromHttpResponse(
}
}

SafeGetTo(get_query_results.arrow_schema, *json, "arrowSchema");
SafeGetTo(get_query_results.arrow_record_batch, *json, "arrowRecordBatch");
if (json->contains("pageRowCount")) {
get_query_results.page_row_count = GetNumberFromJson(*json, "pageRowCount");
}

GetQueryResultsResponse response;
response.http_response = http_response;
response.get_query_results = get_query_results;
Expand Down
35 changes: 35 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/job_response.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,35 @@ class CancelJobResponse {
BigQueryHttpResponse http_response;
};

struct ArrowSchema {
std::string serialized_schema;

std::string DebugString(absl::string_view name,
TracingOptions const& options = {},
int indent = 0) const;
};
void to_json(nlohmann::json& j, ArrowSchema const& a);
void from_json(nlohmann::json const& j, ArrowSchema& a);
inline bool operator==(ArrowSchema const& lhs, ArrowSchema const& rhs) {
return lhs.serialized_schema == rhs.serialized_schema;
}

struct ArrowRecordBatch {
std::string serialized_record_batch;
std::int64_t row_count = 0;

std::string DebugString(absl::string_view name,
TracingOptions const& options = {},
int indent = 0) const;
};
void to_json(nlohmann::json& j, ArrowRecordBatch const& a);
void from_json(nlohmann::json const& j, ArrowRecordBatch& a);
inline bool operator==(ArrowRecordBatch const& lhs,
ArrowRecordBatch const& rhs) {
return lhs.serialized_record_batch == rhs.serialized_record_batch &&
lhs.row_count == rhs.row_count;
}

struct PostQueryResults {
std::string DebugString(absl::string_view name,
TracingOptions const& options = {},
Expand All @@ -117,6 +146,9 @@ struct PostQueryResults {
std::vector<ErrorProto> errors;
SessionInfo session_info;
DmlStats dml_stats;
ArrowSchema arrow_schema;
ArrowRecordBatch arrow_record_batch;
std::int64_t page_row_count = 0;
};
void to_json(nlohmann::json& j, PostQueryResults const& q);
void from_json(nlohmann::json const& j, PostQueryResults& q);
Expand Down Expand Up @@ -153,6 +185,9 @@ struct GetQueryResults {

std::vector<RowData> rows;
std::vector<ErrorProto> errors;
ArrowSchema arrow_schema;
ArrowRecordBatch arrow_record_batch;
std::int64_t page_row_count = 0;

std::string DebugString(absl::string_view name,
TracingOptions const& options = {},
Expand Down
36 changes: 32 additions & 4 deletions google/cloud/bigquery/v2/minimal/internal/job_response_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2009,7 +2009,10 @@ TEST(QueryResponseTest, DebugString) {
R"( rounding_mode { value: "" } range_element_type { type: "" } } })"
R"( job_reference { project_id: "p123" job_id: "j123" location: "useast" })"
R"( session_info { session_id: "123" } dml_stats { inserted_row_count: 10)"
R"( deleted_row_count: 10 updated_row_count: 10 } } })");
R"( deleted_row_count: 10 updated_row_count: 10 })"
R"( arrow_schema { serialized_schema: "testing_schema_data" })"
R"( arrow_record_batch { serialized_record_batch: "testing_batch_data" row_count: 10 })"
R"( page_row_count: 10 } })");

EXPECT_EQ(
response->DebugString(
Expand All @@ -2029,7 +2032,10 @@ TEST(QueryResponseTest, DebugString) {
R"( rounding_mode { value: "" } range_element_type { type: "" } } })"
R"( job_reference { project_id: "p123" job_id: "j123" location: "useast" })"
R"( session_info { session_id: "123" } dml_stats {)"
R"( inserted_row_count: 10 deleted_row_count: 10 updated_row_count: 10 } } })");
R"( inserted_row_count: 10 deleted_row_count: 10 updated_row_count: 10 })"
R"( arrow_schema { serialized_schema: "testing...<truncated>..." })"
R"( arrow_record_batch { serialized_record_batch: "testing...<truncated>..." row_count: 10 })"
R"( page_row_count: 10 } })");

EXPECT_EQ(response->DebugString("QueryResponse", TracingOptions{}.SetOptions(
"single_line_mode=F")),
Expand Down Expand Up @@ -2108,6 +2114,14 @@ TEST(QueryResponseTest, DebugString) {
deleted_row_count: 10
updated_row_count: 10
}
arrow_schema {
serialized_schema: "testing_schema_data"
}
arrow_record_batch {
serialized_record_batch: "testing_batch_data"
row_count: 10
}
page_row_count: 10
}
})");
}
Expand Down Expand Up @@ -2166,7 +2180,10 @@ TEST(GetQueryResultsResponseTest, DebugString) {
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
R"( rounding_mode { value: "" } range_element_type { type: "" } } })"
R"( job_reference { project_id: "p123" job_id: "j123" location: "useast" } } })");
R"( job_reference { project_id: "p123" job_id: "j123" location: "useast" })"
R"( arrow_schema { serialized_schema: "testing_schema_data" })"
R"( arrow_record_batch { serialized_record_batch: "testing_batch_data" row_count: 10 })"
R"( page_row_count: 10 } })");

EXPECT_EQ(
response->DebugString(
Expand All @@ -2185,7 +2202,10 @@ TEST(GetQueryResultsResponseTest, DebugString) {
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
R"( rounding_mode { value: "" } range_element_type { type: "" } } })"
R"( job_reference { project_id: "p123" job_id: "j123" location: "useast" } } })");
R"( job_reference { project_id: "p123" job_id: "j123" location: "useast" })"
R"( arrow_schema { serialized_schema: "testing...<truncated>..." })"
R"( arrow_record_batch { serialized_record_batch: "testing...<truncated>..." row_count: 10 })"
R"( page_row_count: 10 } })");

EXPECT_EQ(
response->DebugString("GetQueryResultsResponse",
Expand Down Expand Up @@ -2258,6 +2278,14 @@ TEST(GetQueryResultsResponseTest, DebugString) {
job_id: "j123"
location: "useast"
}
arrow_schema {
serialized_schema: "testing_schema_data"
}
arrow_record_batch {
serialized_record_batch: "testing_batch_data"
row_count: 10
}
page_row_count: 10
}
})");
}
Expand Down
Loading
Loading