ARROW-3982: [C++] Allow "binary" input in simple JSON format - #3222
ARROW-3982: [C++] Allow "binary" input in simple JSON format#3222pitrou wants to merge 1 commit into
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3222 +/- ##
==========================================
+ Coverage 88.52% 89.67% +1.14%
==========================================
Files 536 478 -58
Lines 72407 68438 -3969
==========================================
- Hits 64099 61371 -2728
+ Misses 8203 7067 -1136
+ Partials 105 0 -105
Continue to review full report at Codecov.
|
fsaintjacques
left a comment
fsaintjacques
left a comment
There was a problem hiding this comment.
LGTM, minor style refactor proposed.
There was a problem hiding this comment.
I'd refactor this function with a simple switch (if possible?) on Json type and hide the string_view parsing in a static inline function.
switch(json_type) {
case Json::NULL: return AppendNull();
case Json::STRING: {
util::string_view view;
ARROW_RETURN_NOT_OK(JsonStringToViewOfFixedSize(json_obj, fixed_size, &view));
return builder_->Append(view);
}
default: return Status::Invalid(...);
}This is a style preference, but I think it improves reading since there's less control flow involved.
There was a problem hiding this comment.
The current idiom (a chain of ifs) is used elsewhere in the module, though. So we'd have to change other occurrences as well. IMHO there's not much benefit.
There was a problem hiding this comment.
I think it's OK to leave the code as is since it's similar to the rest of the file. If future refactoring is needed then whoever is working on it is free to do this code scrubbing
There was a problem hiding this comment.
I think it's OK to leave the code as is since it's similar to the rest of the file. If future refactoring is needed then whoever is working on it is free to do this code scrubbing
|
Hm looks like this got touched by ce9c6e3. I will rebase quickly |
Since rapidjson doesn't validate UTF8 by default, we can represent many arbitrary binary bytes (except control characters < 0x20) in the JSON input.
de36a89 to
5aaa5ed
Compare
| if (json_obj.IsString()) { | ||
| auto view = util::string_view(json_obj.GetString(), json_obj.GetStringLength()); | ||
| if (view.length() != static_cast<size_t>(builder_->byte_width())) { | ||
| std::stringstream ss; |
There was a problem hiding this comment.
This can be simplified now :-)
4 participants
Since rapidjson doesn't validate UTF8 by default, we can represent arbitrary binary bytes in the JSON input (bytes < 0x20 have to be represented as unicode escapes).