Copilot commented on code in PR #50945:
URL: https://github.com/apache/arrow/pull/50945#discussion_r3900097947
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -17,105 +17,71 @@
#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];
- }
- 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;
+static Status ConsumeDocument(simdjson::ondemand::document_stream::iterator&
it) {
+ auto document_result =
+ internal::ResolveSimdjsonResult(*it, "Failed to get JSON document");
+ ARROW_RETURN_NOT_OK(document_result.status());
+
+ auto document = *document_result;
+
+ auto value_result =
+ internal::ResolveSimdjsonResult(document.get_value(), "Failed to get
JSON value");
+ ARROW_RETURN_NOT_OK(value_result.status());
+
+ auto value = *value_result;
+ return internal::ConsumeJsonValue(value);
+}
+
+static size_t ConsumeWholeObject(const simdjson::padded_string& input) {
+ if (input.size() == 0) {
+ return 0;
}
- 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;
+
+ simdjson::ondemand::parser parser;
+ simdjson::ondemand::document_stream stream;
+
+ if (parser.iterate_many(input).get(stream) != simdjson::SUCCESS) {
+ return std::string_view::npos;
}
Review Comment:
The new boundary detection relies on simdjson's document_stream
(iterate_many) to compute the first document length. This is stricter than the
prior RapidJSON stop-when-done approach: if the buffer contains a valid record
followed by invalid bytes, simdjson may reject the stream and prevent emitting
the valid prefix (as reflected by the updated chunker_test). That appears to
conflict with the issue/PR goal of preserving chunking/error-propagation
behavior and with the PR description’s “structural scanning + validation”
approach. Consider switching to true structural boundary scanning to find a
candidate end position, then validating just that candidate (e.g., via
internal::ValidateJsonDocument) so trailing invalid data doesn’t suppress
already-complete records.
##########
python/pyarrow/tests/test_json.py:
##########
@@ -150,8 +150,7 @@ def test_block_sizes(self):
for newlines_in_values in [False, True]:
parse_options.newlines_in_values = newlines_in_values
read_options.block_size = 4
- with pytest.raises(ValueError,
- match="try to increase block size"):
+ with pytest.raises(ValueError):
self.read_bytes(data, read_options=read_options,
parse_options=parse_options)
Review Comment:
This test now accepts any ValueError, which weakens coverage and can hide
regressions. Since other Python JSON tests still assert the small-block-size
failure message, consider keeping the match for the line-delimited chunker case
(newlines_in_values=False) while allowing the parsing-based chunker case to
vary.
--
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]
