taepper commented on code in PR #51038:
URL: https://github.com/apache/arrow/pull/51038#discussion_r4094391987
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2837,6 +2837,9 @@ function(build_simdjson)
prepare_fetchcontent()
+ # Keep simdjson's threading configuration consistent with Arrow's,
+ # which is required for Emscripten where Arrow threading is disabled.
+ set(SIMDJSON_ENABLE_THREADS ${ARROW_ENABLE_THREADING})
Review Comment:
https://github.com/simdjson/simdjson/blob/master/doc/iterate_many.md
> Thread support is only active if thread supported is detected in which
case the macro SIMDJSON_THREADS_ENABLED is set. You can also manually pass
SIMDJSON_THREADS_ENABLED=1 flag to the library. Otherwise the library runs in
single-thread mode.
Note the code section for this auto-detection:
```
// Is threading enabled?
#if defined(_REENTRANT) || defined(_MT)
#ifndef SIMDJSON_THREADS_ENABLED
#define SIMDJSON_THREADS_ENABLED
#endif
#endif
```
So even if we do not set the cmake option `SIMDJSON_ENABLE_THREADS`, we can
still get threading enabled
##########
cpp/src/arrow/json/parser.cc:
##########
@@ -762,102 +744,205 @@ class HandlerBase : public BlockParser,
}
protected:
- template <typename Handler, typename Stream>
- Status DoParse(Handler& handler, Stream&& json, size_t json_size) {
- constexpr auto parse_flags = rj::kParseIterativeFlag |
rj::kParseNanAndInfFlag |
- rj::kParseStopWhenDoneFlag |
- rj::kParseNumbersAsStringsFlag;
-
- rj::Reader reader;
- // ensure that the loop can exit when the block too large.
- for (; num_rows_ < std::numeric_limits<int32_t>::max(); ++num_rows_) {
- auto ok = reader.Parse<parse_flags>(json, handler);
- switch (ok.Code()) {
- case rj::kParseErrorNone:
- // parse the next object
- continue;
- case rj::kParseErrorDocumentEmpty:
- if (json.Tell() < json_size) {
- return ParseError(rj::GetParseError_En(ok.Code()));
- }
- // parsed all objects, finish
- return Status::OK();
- case rj::kParseErrorTermination:
- // handler emitted an error
- return handler.Error();
- default:
- // rj emitted an error
- return ParseError(rj::GetParseError_En(ok.Code()), " in row ",
num_rows_);
+ Status Parse(const std::shared_ptr<Buffer>& json) override {
+ RETURN_NOT_OK(ReserveScalarStorage(json->size()));
+
+ const std::string_view input(reinterpret_cast<const char*>(json->data()),
+ json->size());
+
+ const int64_t input_size = input.size();
+ if (internal::ConsumeJsonWhitespace(input, /*trailing=*/false) ==
input_size) {
+ return Status::OK();
+ }
+
+ auto parse = [&](const auto& input) -> Status {
+ ARROW_ASSIGN_OR_RAISE(auto stream,
arrow::internal::ResolveSimdjsonResult(
+ parser_.iterate_many(input),
+ "Failed to create JSON document
stream"));
+
+ for (auto document_result : stream) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto document,
+ arrow::internal::ResolveSimdjsonResult(
+ document_result, "Failed to iterate JSON document stream"));
+
+ if (num_rows_ == std::numeric_limits<int32_t>::max()) {
+ return Status::Invalid("Row count overflowed int32_t");
+ }
+
+ ARROW_ASSIGN_OR_RAISE(
+ auto value,
+ arrow::internal::ResolveSimdjsonResult(
+ document.get_value(), "JSON parse error: Failed to get JSON
value"));
+
+ RETURN_NOT_OK(ParseValue(value));
+
+ ++num_rows_;
}
+
+ if (stream.truncated_bytes() != 0) {
+ return ParseError("JSON document was truncated");
+ }
Review Comment:
I think `truncated_bytes` can also have problems when we partial utf-8
characters at the end of the input.
Also conceptually, we did not use `truncated_bytes` in the json chunker. It
is now not obvious to me why we use simdjson's api in a different way for the
parser. To me this will make the code more brittle and we should be consistent
with our api usage.
Note that we could replace the `FindLast` method in the chunker with a
single call to `truncated_bytes()`. But this can lead to some weird behavior
with partial utf-8 characters at the end of the stream
--
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]
