-
Notifications
You must be signed in to change notification settings - Fork 4.2k
GH-50654: [C++] Support simdjson without exceptions #50672
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,9 @@ class ObjectParser::Impl { | |
| padded_json_ = simdjson::padded_string(json); | ||
|
|
||
| // Store parsed document | ||
| document_ = parser_.iterate(padded_json_); | ||
| if (auto error = parser_.iterate(padded_json_).get(document_)) { | ||
| return Status::Invalid("JSON parse error: ", simdjson::error_message(error)); | ||
| } | ||
|
|
||
| // Validate root is an object | ||
| auto object = document_.get_object(); | ||
|
|
@@ -69,7 +71,12 @@ class ObjectParser::Impl { | |
| "': ", simdjson::error_message(str_result.error())); | ||
| } | ||
|
|
||
| return std::string(str_result.value()); | ||
| std::string_view str; | ||
| if (auto error = std::move(str_result).get(str)) { | ||
| return Status::Invalid("Error getting string for key '", key, | ||
| "': ", simdjson::error_message(error)); | ||
| } | ||
| return std::string(str); | ||
| } | ||
|
|
||
| Result<std::unordered_map<std::string, std::string>> GetStringMap() { | ||
|
|
@@ -80,13 +87,10 @@ class ObjectParser::Impl { | |
| auto object = document_.get_object(); | ||
|
|
||
| for (auto field : object) { | ||
| auto key_result = field.unescaped_key(); | ||
|
|
||
| auto key = key_result.value(); | ||
|
|
||
| if (key_result.error()) { | ||
| return Status::Invalid("Error getting value for key '", std::string(key), | ||
| "': ", simdjson::error_message(key_result.error())); | ||
| std::string_view key; | ||
| if (auto error = field.unescaped_key().get(key)) { | ||
| return Status::Invalid("Error getting object key: ", | ||
| simdjson::error_message(error)); | ||
| } | ||
|
|
||
| auto value = field.value(); | ||
|
|
@@ -102,7 +106,13 @@ class ObjectParser::Impl { | |
| "': (code=", static_cast<int>(str_result.error()), ")"); | ||
| } | ||
|
|
||
| map.emplace(std::string(key), std::string(str_result.value())); | ||
| std::string_view str; | ||
| if (auto error = std::move(str_result).get(str)) { | ||
| return Status::Invalid("Error getting value for key '", std::string(key), | ||
| "': ", simdjson::error_message(error)); | ||
| } | ||
|
|
||
| map.emplace(std::string(key), std::string(str)); | ||
|
Comment on lines
90
to
+115
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, first asserting the value (L110), then type-checking the value (L98), is cleaner IMO. (It also works the other way around because simdjson provides overloads for many methods on its result type) Note that for asserting the value and type-checking the value #50653 adds helper methods we might want to reuse |
||
| } | ||
|
|
||
| return map; | ||
|
|
@@ -132,7 +142,13 @@ class ObjectParser::Impl { | |
| "': ", simdjson::error_message(bool_result.error())); | ||
| } | ||
|
|
||
| return bool_result.value(); | ||
| bool value; | ||
| if (auto error = std::move(bool_result).get(value)) { | ||
| return Status::Invalid("Error getting bool for key '", key, | ||
| "': ", simdjson::error_message(error)); | ||
| } | ||
|
Comment on lines
+145
to
+149
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #50653 adds that already returns |
||
|
|
||
| return value; | ||
| } | ||
|
|
||
| private: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#50653 adds
that already returns
arrow::Results instead of error codes. This might simplify these type-castsUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions! I opened #50693 to track this cleanup after #50672 is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, #50653 is merged now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to write after this PR is merged. Referenced the wrong PR. Fixed it.