pitrou commented on code in PR #50945:
URL: https://github.com/apache/arrow/pull/50945#discussion_r3842208401
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -22,100 +22,91 @@
#include <utility>
#include <vector>
-#include "arrow/json/rapidjson_defs.h"
-#include "rapidjson/reader.h"
+#include <simdjson.h>
#include "arrow/buffer.h"
#include "arrow/json/options.h"
#include "arrow/util/logging_internal.h"
+#include "arrow/util/simdjson_internal.h"
namespace arrow {
using std::string_view;
Review Comment:
Can we remove this if it's not useful anymore?
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -22,100 +22,91 @@
#include <utility>
#include <vector>
-#include "arrow/json/rapidjson_defs.h"
-#include "rapidjson/reader.h"
+#include <simdjson.h>
#include "arrow/buffer.h"
#include "arrow/json/options.h"
#include "arrow/util/logging_internal.h"
+#include "arrow/util/simdjson_internal.h"
namespace arrow {
using std::string_view;
namespace json {
-namespace rj = arrow::rapidjson;
-
-static size_t ConsumeWhitespace(string_view view) {
-#ifdef RAPIDJSON_SIMD
- auto data = view.data();
- auto nonws_begin = rj::SkipWhitespace_SIMD(data, data + view.size());
- return nonws_begin - data;
-#else
- auto ws_count = view.find_first_not_of(" \t\r\n");
- if (ws_count == string_view::npos) {
+static size_t ConsumeWhitespace(std::string_view view) {
+ const auto ws_count = view.find_first_not_of(" \t\r\n");
+ if (ws_count == std::string_view::npos) {
return view.size();
- } else {
- return ws_count;
}
-#endif
+ return ws_count;
}
-/// RapidJson custom stream for reading JSON stored in multiple buffers
-/// http://rapidjson.org/md_doc_stream.html#CustomStream
-class MultiStringStream {
- public:
- using Ch = char;
- explicit MultiStringStream(std::vector<string_view> strings)
- : strings_(std::move(strings)) {
- std::reverse(strings_.begin(), strings_.end());
- }
- explicit MultiStringStream(const BufferVector& buffers) :
strings_(buffers.size()) {
- for (size_t i = 0; i < buffers.size(); ++i) {
- strings_[i] = string_view(*buffers[i]);
- }
- std::reverse(strings_.begin(), strings_.end());
- }
- char Peek() const {
- if (strings_.size() == 0) return '\0';
- return strings_.back()[0];
- }
- char Take() {
- if (strings_.size() == 0) return '\0';
- char taken = strings_.back()[0];
- if (strings_.back().size() == 1) {
- strings_.pop_back();
- } else {
- strings_.back() = strings_.back().substr(1);
- }
- ++index_;
- return taken;
- }
- size_t Tell() { return index_; }
- void Put(char) { ARROW_LOG(FATAL) << "not implemented"; }
- void Flush() { ARROW_LOG(FATAL) << "not implemented"; }
- char* PutBegin() {
- ARROW_LOG(FATAL) << "not implemented";
- return nullptr;
+static size_t ConsumeWholeObject(string_view input) {
+ if (input.empty()) {
+ return 0;
}
- size_t PutEnd(char*) {
- ARROW_LOG(FATAL) << "not implemented";
+
+ const size_t start = ConsumeWhitespace(input);
+ if (start >= input.size()) {
return 0;
}
- private:
- size_t index_ = 0;
- std::vector<string_view> strings_;
-};
+ int depth = 0;
+ bool in_string = false;
+ bool escape_next = false;
+ bool started = false;
+
+ for (size_t i = start; i < input.size(); ++i) {
+ const char c = input[i];
+
+ if (escape_next) {
+ escape_next = false;
+ continue;
+ }
+
+ if (c == '\\' && in_string) {
+ escape_next = true;
+ continue;
+ }
Review Comment:
This does not account for unicode escapes. Do we have to parse JSON by hand
like this?
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -124,40 +115,76 @@ namespace {
// and uses actual JSON parsing to delimit them.
class ParsingBoundaryFinder : public BoundaryFinder {
public:
- Status FindFirst(string_view partial, string_view block, int64_t* out_pos)
override {
- auto length = ConsumeWholeObject(MultiStringStream({partial, block}));
+ Status FindFirst(std::string_view partial, std::string_view block,
+ int64_t* out_pos) override {
+ std::string combined;
+ combined.reserve(partial.size() + block.size());
+ combined.append(partial);
+ combined.append(block);
Review Comment:
We should only concatenate if both substrings are non-empty.
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -124,40 +115,76 @@ namespace {
// and uses actual JSON parsing to delimit them.
class ParsingBoundaryFinder : public BoundaryFinder {
public:
- Status FindFirst(string_view partial, string_view block, int64_t* out_pos)
override {
- auto length = ConsumeWholeObject(MultiStringStream({partial, block}));
+ Status FindFirst(std::string_view partial, std::string_view block,
+ int64_t* out_pos) override {
+ std::string combined;
+ combined.reserve(partial.size() + block.size());
+ combined.append(partial);
+ combined.append(block);
+
+ const size_t start = ConsumeWhitespace(combined);
+ if (start < combined.size() && combined[start] != '{' && combined[start]
!= '[') {
+ return Status::Invalid("JSON parse error: Invalid value");
+ }
+
+ const auto length = ConsumeWholeObject(combined);
+
if (length == string_view::npos) {
*out_pos = -1;
} else if (ARROW_PREDICT_FALSE(length < partial.size())) {
- return Status::Invalid("JSON chunk error: invalid data at end of
document");
+ return Status::Invalid("JSON parse error: Invalid value");
} else {
DCHECK_LE(length, partial.size() + block.size());
*out_pos = static_cast<int64_t>(length - partial.size());
}
+
return Status::OK();
}
Status FindLast(std::string_view block, int64_t* out_pos) override {
const size_t block_length = block.size();
size_t consumed_length = 0;
+
+ if (block_length > 0) {
+ const size_t start = ConsumeWhitespace(block);
+ if (start < block.size() && block[start] != '{' && block[start] != '[') {
+ return Status::Invalid("JSON parse error: Invalid value");
+ }
+ }
+
while (consumed_length < block_length) {
- rj::MemoryStream ms(reinterpret_cast<const char*>(block.data()),
block.size());
- using InputStream = rj::EncodedInputStream<rj::UTF8<>, rj::MemoryStream>;
- auto length = ConsumeWholeObject(InputStream(ms));
+ const auto length = ConsumeWholeObject(block);
+
if (length == string_view::npos || length == 0) {
- // found incomplete object or block is empty
+ const size_t start = ConsumeWhitespace(block);
+
+ if (start < block.size()) {
+ const char first_char = block[start];
+
+ if (first_char != '{' && first_char != '[') {
+ const size_t remaining_len = block.size() - start;
+
+ if (remaining_len > 1 || (first_char != '}' && first_char != ']'))
{
+ return Status::Invalid("JSON parse error: Invalid value");
+ }
+ }
+ }
Review Comment:
Please add comments explaining what this does and why it is necessary.
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -124,40 +115,76 @@ namespace {
// and uses actual JSON parsing to delimit them.
class ParsingBoundaryFinder : public BoundaryFinder {
public:
- Status FindFirst(string_view partial, string_view block, int64_t* out_pos)
override {
- auto length = ConsumeWholeObject(MultiStringStream({partial, block}));
+ Status FindFirst(std::string_view partial, std::string_view block,
+ int64_t* out_pos) override {
+ std::string combined;
+ combined.reserve(partial.size() + block.size());
+ combined.append(partial);
+ combined.append(block);
+
+ const size_t start = ConsumeWhitespace(combined);
+ if (start < combined.size() && combined[start] != '{' && combined[start]
!= '[') {
+ return Status::Invalid("JSON parse error: Invalid value");
+ }
+
+ const auto length = ConsumeWholeObject(combined);
+
if (length == string_view::npos) {
*out_pos = -1;
} else if (ARROW_PREDICT_FALSE(length < partial.size())) {
- return Status::Invalid("JSON chunk error: invalid data at end of
document");
+ return Status::Invalid("JSON parse error: Invalid value");
Review Comment:
Why not keep the error message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
