Copilot commented on code in PR #50945:
URL: https://github.com/apache/arrow/pull/50945#discussion_r3901996248
##########
cpp/src/arrow/json/chunker_test.cc:
##########
@@ -264,9 +264,15 @@ TEST(ChunkerTest, Errors) {
std::string parts[] = {R"({"a":0})", "}", R"({"a":1})"};
auto chunker = MakeChunker(true);
std::shared_ptr<Buffer> whole, rest, completion;
+
ASSERT_OK(chunker->Process(Buffer::FromString(parts[0] + parts[1]), &whole,
&rest));
- ASSERT_EQ(std::string_view(*whole), parts[0]);
- ASSERT_EQ(std::string_view(*rest), parts[1]);
+
+ // simdjson rejects the malformed stream as a whole, so no complete chunk
+ // is emitted before the trailing invalid data.
+ ASSERT_TRUE(whole);
+ ASSERT_EQ(std::string_view(*whole), "");
+ ASSERT_EQ(std::string_view(*rest), parts[0] + parts[1]);
Review Comment:
The updated test asserts that a valid JSON prefix is no longer emitted when
the same buffer contains trailing invalid bytes ("simdjson rejects the
malformed stream as a whole"). This appears to change
chunking/error-propagation semantics, which conflicts with the PR/issue
requirement to preserve existing chunking behavior. Either adjust the
implementation to still detect/emit a complete prefix (e.g., structural
boundary scan + validate only the candidate prefix) or update the PR
description/issue expectations to explicitly document the behavior change.
##########
cpp/src/arrow/json/chunker.cc:
##########
@@ -124,40 +86,114 @@ 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();
+ }
Review Comment:
`ParsingBoundaryFinder::FindFirst/FindLast` now copies input into
`simdjson::padded_string` (and in `FindFirst` potentially concatenates
`partial+block`) on every call. For large `ReadOptions::block_size` this adds
an extra full-buffer allocation/copy per chunking step and can materially
impact throughput. Consider reusing a scratch padded buffer and a
`simdjson::ondemand::parser` as members of `ParsingBoundaryFinder`, and only
(re)allocating when the required capacity grows (or using `padded_string_view`
when the underlying buffer is already padded).
--
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]
