This is an automated email from the ASF dual-hosted git repository.
pitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 2d692929b7f GH-51252: [C++] Reduce boilerplate in OpaqueType JSON
deserialization (#51253)
2d692929b7f is described below
commit 2d692929b7f1c55e9b213aa16e9c4dcc005eecf3
Author: Aaditya Srinivasan <[email protected]>
AuthorDate: Wed Sep 9 13:10:28 2026 +0530
GH-51252: [C++] Reduce boilerplate in OpaqueType JSON deserialization
(#51253)
### Rationale for this change
This revisits the implementation based on the review feedback from
https://github.com/apache/arrow/pull/50905#issuecomment-5393735829 and uses
Arrow's existing `JsonObjectParser` helper to provide a higher-level interface
over simdjson.
### Changes
- Replace manual simdjson On-Demand parsing in `OpaqueType::Deserialize()`
with `JsonObjectParser`.
- Use `JsonObjectParser::GetString()` to retrieve the required `type_name`
and `vendor_name` fields.
- Remove manual JSON object iteration, key handling, and explicit JSON type
checks.
- Update `OpaqueType::Deserialize` tests to verify invalid input is
rejected without relying on exact error messages.
Fixes: #51252
* GitHub Issue: #51252
Authored-by: Aaditya Srinivasan <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
cpp/src/arrow/extension/opaque.cc | 74 ++--------------------------------
cpp/src/arrow/extension/opaque_test.cc | 35 ++++++----------
2 files changed, 17 insertions(+), 92 deletions(-)
diff --git a/cpp/src/arrow/extension/opaque.cc
b/cpp/src/arrow/extension/opaque.cc
index 2dae9ef5678..57eb05b9e5c 100644
--- a/cpp/src/arrow/extension/opaque.cc
+++ b/cpp/src/arrow/extension/opaque.cc
@@ -63,77 +63,11 @@ std::string OpaqueType::Serialize() const {
Result<std::shared_ptr<DataType>> OpaqueType::Deserialize(
std::shared_ptr<DataType> storage_type, const std::string&
serialized_data) const {
- simdjson::padded_string padded_json(serialized_data);
- simdjson::ondemand::parser parser;
-
- ARROW_ASSIGN_OR_RAISE(auto document,
-
internal::ResolveSimdjsonResult(parser.iterate(padded_json),
- "Failed to parse
JSON"));
-
- ARROW_ASSIGN_OR_RAISE(auto object,
- internal::ResolveSimdjsonResult(document.get_object(),
- "Failed to get JSON
object"));
-
- std::string type_name;
- std::string vendor_name;
- bool has_type_name = false;
- bool has_vendor_name = false;
-
- for (auto field_result : object) {
- ARROW_ASSIGN_OR_RAISE(auto field, internal::ResolveSimdjsonResult(
- field_result, "Failed to iterate
JSON object"));
-
- ARROW_ASSIGN_OR_RAISE(
- auto key, internal::ResolveSimdjsonResult(field.unescaped_key(),
- "Failed to get JSON object
key"));
-
- auto value = field.value();
-
- if (key == "type_name") {
- has_type_name = true;
-
- ARROW_ASSIGN_OR_RAISE(auto type,
- internal::ResolveSimdjsonResult(
- value.type(), "Failed to determine type_name
JSON type"));
-
- if (type != simdjson::ondemand::json_type::string) {
- return Status::Invalid(
- "Invalid serialized JSON data for OpaqueType: type_name is not a
string");
- }
-
- ARROW_ASSIGN_OR_RAISE(
- auto name,
- internal::ResolveSimdjsonResult(value.get_string(), "Failed to get
type_name"));
- type_name = std::string(name);
-
- } else if (key == "vendor_name") {
- has_vendor_name = true;
-
- ARROW_ASSIGN_OR_RAISE(
- auto type, internal::ResolveSimdjsonResult(
- value.type(), "Failed to determine vendor_name JSON
type"));
-
- if (type != simdjson::ondemand::json_type::string) {
- return Status::Invalid(
- "Invalid serialized JSON data for OpaqueType: vendor_name is not a
string");
- }
-
- ARROW_ASSIGN_OR_RAISE(auto name,
- internal::ResolveSimdjsonResult(value.get_string(),
- "Failed to get
vendor_name"));
- vendor_name = std::string(name);
- }
- }
-
- if (!has_type_name) {
- return Status::Invalid(
- "Invalid serialized JSON data for OpaqueType: missing type_name");
- }
+ internal::JsonObjectParser parser;
+ RETURN_NOT_OK(parser.Parse(serialized_data));
- if (!has_vendor_name) {
- return Status::Invalid(
- "Invalid serialized JSON data for OpaqueType: missing vendor_name");
- }
+ ARROW_ASSIGN_OR_RAISE(auto type_name, parser.GetString("type_name"));
+ ARROW_ASSIGN_OR_RAISE(auto vendor_name, parser.GetString("vendor_name"));
return opaque(std::move(storage_type), std::move(type_name),
std::move(vendor_name));
}
diff --git a/cpp/src/arrow/extension/opaque_test.cc
b/cpp/src/arrow/extension/opaque_test.cc
index ac093ddfe07..7c7c2bfa4b0 100644
--- a/cpp/src/arrow/extension/opaque_test.cc
+++ b/cpp/src/arrow/extension/opaque_test.cc
@@ -127,28 +127,19 @@ TEST(OpaqueType, Deserialize) {
auto type = internal::checked_pointer_cast<extension::OpaqueType>(
extension::opaque(null(), "type", "vendor"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, testing::HasSubstr("Failed to parse
JSON"),
- type->Deserialize(null(), R"()"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid,
- testing::HasSubstr("Failed to get JSON
object"),
- type->Deserialize(null(), R"({)"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid,
- testing::HasSubstr("Failed to get JSON
object"),
- type->Deserialize(null(), R"([])"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, testing::HasSubstr("missing
type_name"),
- type->Deserialize(null(), R"({})"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, testing::HasSubstr("type_name is not a string"),
- type->Deserialize(null(), R"({"type_name": 2, "vendor_name": ""})"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, testing::HasSubstr("type_name is not a string"),
- type->Deserialize(null(), R"({"type_name": null, "vendor_name": ""})"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, testing::HasSubstr("vendor_name is not a string"),
- type->Deserialize(null(), R"({"vendor_name": 2, "type_name": ""})"));
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, testing::HasSubstr("vendor_name is not a string"),
- type->Deserialize(null(), R"({"vendor_name": null, "type_name": ""})"));
+
+ ASSERT_RAISES(Invalid, type->Deserialize(null(), R"()"));
+ ASSERT_RAISES(Invalid, type->Deserialize(null(), R"({)"));
+ ASSERT_RAISES(TypeError, type->Deserialize(null(), R"([])"));
+ ASSERT_RAISES(KeyError, type->Deserialize(null(), R"({})"));
+ ASSERT_RAISES(TypeError,
+ type->Deserialize(null(), R"({"type_name": 2, "vendor_name":
""})"));
+ ASSERT_RAISES(TypeError,
+ type->Deserialize(null(), R"({"type_name": null,
"vendor_name": ""})"));
+ ASSERT_RAISES(TypeError,
+ type->Deserialize(null(), R"({"vendor_name": 2, "type_name":
""})"));
+ ASSERT_RAISES(TypeError,
+ type->Deserialize(null(), R"({"vendor_name": null,
"type_name": ""})"));
}
TEST(OpaqueType, MetadataRoundTrip) {
