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.

Sign up for GitHub

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

fix(bigquery): Fixes parsing of null type values returned from BQ server #14384

Merged
merged 3 commits into from
Jun 27, 2024
Merged

fix(bigquery): Fixes parsing of null type values returned from BQ server #14384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ffaa72
fix(bigquery): Fixes parsing of null type values returned from BQ server
jsrinnn Jun 27, 2024
f1aa468
fix(bigquery): addressed clang-tidy issues
jsrinnn Jun 27, 2024
7ac1eb2
fix(bigquery): Added TODO for cleanup
jsrinnn Jun 27, 2024
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
5 changes: 3 additions & 2 deletions google/cloud/bigquery/v2/minimal/internal/common_v2_resources.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,11 +634,11 @@ void to_json(nlohmann::json& j, ColumnData const& c) {
j = nlohmann::json{{"v", c.value}};
}
void from_json(nlohmann::json const& j, ColumnData& c) {
SafeGetTo(c.value, j, "v");
SafeGetToWithNullable(c.value, c.is_null, j, "v");
Copy link
Contributor

Choose a reason for hiding this comment

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

If c.value was absl::optional<std::string> you could just use overloading on SafeGetTo?

}

bool operator==(ColumnData const& lhs, ColumnData const& rhs) {
return lhs.value == rhs.value;
return (lhs.value == rhs.value && lhs.is_null == rhs.is_null);
}

bool operator==(RowData const& lhs, RowData const& rhs) {
Expand All @@ -651,6 +651,7 @@ std::string ColumnData::DebugString(absl::string_view name,
int indent) const {
return internal::DebugFormatter(name, options, indent)
.StringField("value", value)
.Field("is_null", is_null)
.Build();
}

Expand Down
2 changes: 2 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/common_v2_resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,8 @@ bool operator==(Struct const& lhs, Struct const& rhs);

struct ColumnData {
std::string value;
// TODO(#14387): Use absl::optional instead.
bool is_null{false};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be more idiomatic to use absl::optional<std::string> to represent this. At the very least it would avoid awkward situations like value == "foo" and is_null == true.

std::string DebugString(absl::string_view name,
TracingOptions const& options = {},
int indent = 0) const;
Expand Down
86 changes: 71 additions & 15 deletions google/cloud/bigquery/v2/minimal/internal/common_v2_resources_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,25 @@ TEST(CommonV2ResourcesTest, ColumnDataFromJson) {

ColumnData expected;
expected.value = "12345";
expected.is_null = false;

EXPECT_EQ(expected.value, actual.value);
}

TEST(CommonV2ResourcesTest, ColumnDataFromJsonNull) {
std::string text =
R"({
"v":null
})";
auto json = nlohmann::json::parse(text, nullptr, false);
EXPECT_TRUE(json.is_object());

ColumnData actual;
from_json(json, actual);

ColumnData expected;
expected.value = "";
expected.is_null = true;

EXPECT_EQ(expected.value, actual.value);
}
Expand Down Expand Up @@ -510,69 +529,106 @@ TEST(CommonV2ResourcesTest, RowDataFromJson) {
actual.columns.begin()));
}

TEST(CommonV2ResourcesTest, RowDataFromJsonNullValues) {
std::string text =
R"({
"f":[
{
"v":null
},
{
"v":null
}
]
})";
auto json = nlohmann::json::parse(text, nullptr, false);
EXPECT_TRUE(json.is_object());

RowData actual;
from_json(json, actual);

RowData expected;
expected.columns.push_back(ColumnData{"", true});
expected.columns.push_back(ColumnData{"", true});

EXPECT_TRUE(std::equal(expected.columns.begin(), expected.columns.end(),
actual.columns.begin()));
}

TEST(CommonV2ResourcesTest, ColumnsDebugString) {
ColumnData column_data;
column_data.value = "12345";

EXPECT_EQ(column_data.DebugString("ColumnData", TracingOptions{}),
R"(ColumnData {)"
R"( value: "12345")"
R"( is_null: false)"
R"( })");

EXPECT_EQ(column_data.DebugString("ColumnData",
TracingOptions{}.SetOptions(
"truncate_string_field_longer_than=2")),
R"(ColumnData {)"
R"( value: "12...<truncated>...")"
R"( is_null: false)"
R"( })");

EXPECT_EQ(column_data.DebugString("ColumnData", TracingOptions{}.SetOptions(
"single_line_mode=F")),
R"(ColumnData {
value: "12345"
is_null: false
})");
}

TEST(CommonV2ResourcesTest, RowDataDebugString) {
RowData row_data = MakeRowData();

EXPECT_EQ(row_data.DebugString("RowData", TracingOptions{}),
R"(RowData { columns { value: "col1" })"
R"( columns { value: "col2" } columns {)"
R"( value: "col3" } columns {)"
R"( value: "col4" } columns {)"
R"( value: "col5" } columns { value: "col6" } })");

EXPECT_EQ(row_data.DebugString("RowData",
TracingOptions{}.SetOptions(
"truncate_string_field_longer_than=2")),
R"(RowData { columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." })"
R"( columns { value: "co...<truncated>..." } })");
R"(RowData { columns { value: "col1" is_null: false })"
R"( columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false })"
R"( columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false })"
R"( columns { value: "col6" is_null: false } })");

EXPECT_EQ(
row_data.DebugString(
"RowData",
TracingOptions{}.SetOptions("truncate_string_field_longer_than=2")),
R"(RowData { columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false })"
R"( columns { value: "co...<truncated>..." is_null: false } })");

EXPECT_EQ(row_data.DebugString(
"RowData", TracingOptions{}.SetOptions("single_line_mode=F")),
R"(RowData {
columns {
value: "col1"
is_null: false
}
columns {
value: "col2"
is_null: false
}
columns {
value: "col3"
is_null: false
}
columns {
value: "col4"
is_null: false
}
columns {
value: "col5"
is_null: false
}
columns {
value: "col6"
is_null: false
}
})");
}
Expand Down
36 changes: 24 additions & 12 deletions google/cloud/bigquery/v2/minimal/internal/job_response_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1996,9 +1996,9 @@ TEST(QueryResponseTest, DebugString) {
R"( kind: "query-kind" page_token: "np123")"
R"( total_rows: 1000 total_bytes_processed: 1000 num_dml_affected_rows: 5)"
R"( job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2016,9 +2016,9 @@ TEST(QueryResponseTest, DebugString) {
R"( query_results { kind: "query-k...<truncated>...")"
R"( page_token: "np123" total_rows: 1000 total_bytes_processed: 1000)"
R"( num_dml_affected_rows: 5 job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2045,21 +2045,27 @@ TEST(QueryResponseTest, DebugString) {
rows {
columns {
value: "col1"
is_null: false
}
columns {
value: "col2"
is_null: false
}
columns {
value: "col3"
is_null: false
}
columns {
value: "col4"
is_null: false
}
columns {
value: "col5"
is_null: false
}
columns {
value: "col6"
is_null: false
}
}
schema {
Expand Down Expand Up @@ -2149,9 +2155,9 @@ TEST(GetQueryResultsResponseTest, DebugString) {
R"( kind: "query-kind" etag: "query-etag" page_token: "np123")"
R"( total_rows: 1000 total_bytes_processed: 1000)"
R"( num_dml_affected_rows: 5 job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2168,9 +2174,9 @@ TEST(GetQueryResultsResponseTest, DebugString) {
R"( etag: "query-e...<truncated>..." page_token: "np123")"
R"( total_rows: 1000 total_bytes_processed: 1000)"
R"( num_dml_affected_rows: 5 job_complete: true cache_hit: true)"
R"( rows { columns { value: "col1" } columns { value: "col2" })"
R"( columns { value: "col3" } columns { value: "col4" })"
R"( columns { value: "col5" } columns { value: "col6" } })"
R"( rows { columns { value: "col1" is_null: false } columns { value: "col2" is_null: false })"
R"( columns { value: "col3" is_null: false } columns { value: "col4" is_null: false })"
R"( columns { value: "col5" is_null: false } columns { value: "col6" is_null: false } })"
R"( schema { fields { name: "fname-1" type: "" mode: "fmode")"
R"( description: "" collation: "" default_value_expression: "")"
R"( max_length: 0 precision: 0 scale: 0 categories { } policy_tags { })"
Expand All @@ -2197,21 +2203,27 @@ TEST(GetQueryResultsResponseTest, DebugString) {
rows {
columns {
value: "col1"
is_null: false
}
columns {
value: "col2"
is_null: false
}
columns {
value: "col3"
is_null: false
}
columns {
value: "col4"
is_null: false
}
columns {
value: "col5"
is_null: false
}
columns {
value: "col6"
is_null: false
}
}
schema {
Expand Down
32 changes: 28 additions & 4 deletions google/cloud/bigquery/v2/minimal/internal/json_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ bool SafeGetTo(ResponseType& value, nlohmann::json const& j,
std::string const& key) {
auto i = j.find(key);
if (i != j.end()) {
i->get_to(value);
// BQ sends null type values which crashes get_to() so check for null.
if (!i->is_null()) {
i->get_to(value);
}
return true;
}
return false;
Expand All @@ -70,10 +73,13 @@ bool SafeGetTo(std::shared_ptr<T>& value, nlohmann::json const& j,
std::string const& key) {
auto i = j.find(key);
if (i == j.end()) return false;
if (value == nullptr) {
value = std::make_shared<T>();
// BQ sends null type values which crashes get_to() so check for null.
if (!i->is_null()) {
if (value == nullptr) {
value = std::make_shared<T>();
}
i->get_to(*value);
}
i->get_to(*value);
return true;
}

Expand All @@ -85,6 +91,24 @@ void SafeGetTo(nlohmann::json const& j, std::string const& key, R& (C::*f)(T) &,
(obj.*f)(i->get<T>());
}
}

// Same as SafeGetTo but also returns if value was null.
template <typename ResponseType>
bool SafeGetToWithNullable(ResponseType& value, bool& is_null,
nlohmann::json const& j, std::string const& key) {
auto i = j.find(key);
is_null = false;
if (i != j.end()) {
// BQ sends null type values which crashes get_to() so check for null.
if (!i->is_null()) {
i->get_to(value);
} else {
is_null = true;
}
return true;
}
return false;
}
// NOLINTEND(misc-no-recursion)

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
Expand Down
37 changes: 37 additions & 0 deletions google/cloud/bigquery/v2/minimal/internal/json_utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,43 @@ TEST(JsonUtilsTest, RemoveEmptyObjects) {
EXPECT_EQ(expected, json.dump());
}

TEST(JsonUtilsTest, SafeGetToNullValue) {
auto const* const key = "project_id";
auto constexpr kJsonText = R"({"project_id":null})";
auto json = nlohmann::json::parse(kJsonText, nullptr, false);
EXPECT_TRUE(json.is_object());

std::string val;
EXPECT_TRUE(SafeGetTo(val, json, key));
EXPECT_EQ(val, "");
}

TEST(JsonUtilsTest, SafeGetToWithNullableNullValue) {
auto const* const key = "project_id";
auto constexpr kJsonText = R"({"project_id":null})";
auto json = nlohmann::json::parse(kJsonText, nullptr, false);
EXPECT_TRUE(json.is_object());

std::string val;
bool is_null;
EXPECT_TRUE(SafeGetToWithNullable(val, is_null, json, key));
EXPECT_EQ(val, "");
EXPECT_TRUE(is_null);
}

TEST(JsonUtilsTest, SafeGetToWithNullableNonNull) {
auto const* const key = "project_id";
auto constexpr kJsonText = R"({"project_id":"123"})";
auto json = nlohmann::json::parse(kJsonText, nullptr, false);
EXPECT_TRUE(json.is_object());

std::string val;
bool is_null;
EXPECT_TRUE(SafeGetToWithNullable(val, is_null, json, key));
EXPECT_EQ(val, "123");
EXPECT_FALSE(is_null);
}

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace bigquery_v2_minimal_internal
} // namespace cloud
Expand Down
Loading