Reranko05 commented on code in PR #50945:
URL: https://github.com/apache/arrow/pull/50945#discussion_r3894273024
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -17,105 +17,83 @@
#include "arrow/json/chunker.h"
-#include <algorithm>
#include <string_view>
#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];
+static size_t ConsumeWholeObject(std::string_view input) {
+ if (input.empty()) {
+ return 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;
+
+ const size_t start = ConsumeWhitespace(input);
Review Comment:
I kept `ConsumeWhitespace()` because removing it causes these existing tests
to fail:
* `ChunkerTest/BaseChunkerTest.StraddlingEmpty/0` — whitespace-only partial
data is no longer recognized correctly, so it gets treated as a completed JSON
value.
*
`StreamingReaderTest/StreamingReaderTest.PropagateErrorsNonLinewiseChunker/0` —
changes where the chunk boundary is detected, causing incorrect
`bytes_processed()` and error propagation.
*
`StreamingReaderTest/StreamingReaderTest.PropagateErrorsNonLinewiseChunker/1` —
same boundary/error-propagation issue for the other streaming configuration.
So it appears `ConsumeWhitespace()` is needed to preserve the existing chunk
boundary and error-handling semantics.
--
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]
