kou commented on code in PR #50945:
URL: https://github.com/apache/arrow/pull/50945#discussion_r3908747619
##########
cpp/src/arrow/json/reader_test.cc:
##########
@@ -758,20 +757,26 @@ TEST_P(StreamingReaderTest,
PropagateErrorsNonLinewiseChunker) {
AssertReadNext(reader, &batch);
EXPECT_EQ(reader->bytes_processed(), 9);
ASSERT_BATCHES_EQUAL(*RecordBatchFromJSON(test_schema, "[{\"i\":0}]"),
*batch);
- // Chunker doesn't require newline delimiters, so this should be valid
+
+ // The chunker doesn't require newline delimiters between records.
AssertReadNext(reader, &batch);
EXPECT_EQ(reader->bytes_processed(), 20);
ASSERT_BATCHES_EQUAL(*RecordBatchFromJSON(test_schema, "[{\"i\":1}]"),
*batch);
- EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid,
- ::testing::StartsWith("Invalid: JSON parse
error"),
- reader->ReadNext(&batch));
- EXPECT_EQ(reader->bytes_processed(), 20);
- // Incoming chunker error from ":2}" shouldn't leak through after the first
failure,
- // which is a possibility if async tasks are still outstanding due to
readahead.
+ // Depending on readahead, the malformed record may be reported by either
+ // the parser or the chunker on the next read.
+ auto status = reader->ReadNext(&batch);
+ if (status.ok()) {
+ status = reader->ReadNext(&batch);
+ }
+ ASSERT_FALSE(status.ok());
+ EXPECT_TRUE(status.IsInvalid());
+ EXPECT_THAT(status.ToStringWithoutContextLines(),
+ ::testing::AnyOf(::testing::StartsWith("Invalid: JSON parse
error"),
+ ::testing::StartsWith("Invalid: JSON chunk
error")));
Review Comment:
Can we use `ASSERT_RAISES_WITH_MESSAGE()`?
```suggestion
auto status = reader->ReadNext(&batch);
if (status.ok()) {
ASSERT_RAISES_WITH_MESSAGE(INVALID, ::testing::StartsWith("Invalid: JSON
parse error"), reader->ReadNext(&batch));
} else {
ASSERT_RAISES_WITH_MESSAGE(INVALID, ::testing::StartsWith("Invalid: JSON
chunk error"), status);
}
```
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -124,43 +86,77 @@ 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}));
- if (length == string_view::npos) {
+ Status FindFirst(std::string_view partial, std::string_view block,
+ int64_t* out_pos) override {
+ simdjson::padded_string input;
+
+ if (partial.empty()) {
+ input = simdjson::padded_string(block);
+ } else if (block.empty()) {
+ input = simdjson::padded_string(partial);
+ } else {
+ simdjson::padded_string_builder builder(partial.size() + block.size());
+ builder.append(partial);
+ builder.append(block);
+ input = builder.convert();
+ }
+
+ const std::string_view input_view(input.data(), input.size());
+ const size_t start = ConsumeWhitespace(input_view);
+ if (start < input_view.size() && input_view[start] != '{' &&
+ input_view[start] != '[') {
+ return Status::Invalid("JSON chunk error: invalid data at end of
document");
+ }
Review Comment:
Do we need this check? It seems that this is redundant.
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -124,43 +86,77 @@ 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}));
- if (length == string_view::npos) {
+ Status FindFirst(std::string_view partial, std::string_view block,
+ int64_t* out_pos) override {
+ simdjson::padded_string input;
+
+ if (partial.empty()) {
+ input = simdjson::padded_string(block);
+ } else if (block.empty()) {
+ input = simdjson::padded_string(partial);
+ } else {
+ simdjson::padded_string_builder builder(partial.size() + block.size());
+ builder.append(partial);
+ builder.append(block);
+ input = builder.convert();
+ }
+
+ const std::string_view input_view(input.data(), input.size());
+ const size_t start = ConsumeWhitespace(input_view);
+ if (start < input_view.size() && input_view[start] != '{' &&
+ input_view[start] != '[') {
+ return Status::Invalid("JSON chunk error: invalid data at end of
document");
+ }
+
+ const auto length = ConsumeWholeObject(input);
+
+ if (length == std::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");
} 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;
- 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));
- if (length == string_view::npos || length == 0) {
- // found incomplete object or block is empty
+
+ simdjson::padded_string padded(block);
+ simdjson::ondemand::parser parser;
Review Comment:
Cam we reuse `simdjson::ondemand::parser` in `FindLast()` and
`ConsumeWholeObject()` for performance?
See also:
https://github.com/simdjson/simdjson/blob/master/doc/performance.md#reusing-the-parser-for-maximum-efficiency
##########
python/pyarrow/tests/test_json.py:
##########
@@ -529,9 +528,13 @@ def test_non_linewise_chunker_bad_middle_block(self):
'n': [1]
}
- with pytest.raises(pa.ArrowInvalid,
- match="JSON parse error *"):
+ try:
reader.read_next_batch()
+ except pa.ArrowInvalid:
+ pass
+ else:
+ with pytest.raises(pa.ArrowInvalid):
+ reader.read_next_batch()
Review Comment:
Can we use this?
```python
if self.use_threads:
# TODO: Add a comment why the first read_next_batch() may not raise an
error with threads.
with pytest.raises(pa.ArrowInvalid, match="JSON (parse|chunk) error"):
reader.read_next_batch()
reader.read_next_batch()
else:
with pytest.raises(pa.ArrowInvalid, match="JSON parse error"):
reader.read_next_batch()
```
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -124,43 +86,77 @@ 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}));
- if (length == string_view::npos) {
+ Status FindFirst(std::string_view partial, std::string_view block,
+ int64_t* out_pos) override {
+ simdjson::padded_string input;
+
+ if (partial.empty()) {
+ input = simdjson::padded_string(block);
+ } else if (block.empty()) {
+ input = simdjson::padded_string(partial);
+ } else {
+ simdjson::padded_string_builder builder(partial.size() + block.size());
+ builder.append(partial);
+ builder.append(block);
+ input = builder.convert();
+ }
+
+ const std::string_view input_view(input.data(), input.size());
+ const size_t start = ConsumeWhitespace(input_view);
+ if (start < input_view.size() && input_view[start] != '{' &&
+ input_view[start] != '[') {
+ return Status::Invalid("JSON chunk error: invalid data at end of
document");
+ }
+
+ const auto length = ConsumeWholeObject(input);
+
+ if (length == std::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");
} 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;
- 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));
- if (length == string_view::npos || length == 0) {
- // found incomplete object or block is empty
+
+ simdjson::padded_string padded(block);
Review Comment:
Can we use `simdjson::padded_string_view` if possible by introducing our
internal buffer?
See also:
https://github.com/simdjson/simdjson/blob/master/doc/performance.md#reusing-string-buffers
--
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]
