GH-50944: [C++] Replace RapidJSON with simdjson in JSON chunker by Reranko05 · Pull Request #50945 · apache/arrow

Reranko05 · 2026-08-21T13:39:30Z

Rationale for this change

This PR continues the simdjson migration by replacing the RapidJSON-based JSON boundary detection used by the JSON chunker.

The existing implementation uses RapidJSON's streaming parser to identify complete JSON values. This change replaces that logic with the simdjson streaming parser.

Are these changes tested?

Yes, by existing tests.

Are there any user-facing changes?

No, except perhaps slight differences in error message and timing of error reporting when reading from an invalid JSON stream (i.e. there may be cases where a JSON parse error is reported earlier or later while iterating over RecordBatches).

GitHub Issue: [C++] Replace RapidJSON with simdjson in JSON chunker #50944

Reranko05 · 2026-08-21T14:05:22Z

While working on this migration, @rok's earlier implementation rok#47 of the simdjson-based JSON chunker. It was very helpful. Thanks :)

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

pitrou

I don't understand why this is parsing JSON by hand?

It seems that we might be able to use simdjson::ondemand::parser::iterate_many.

Reranko05 · 2026-08-24T10:05:59Z

I don't understand why this is parsing JSON by hand?
It seems that we might be able to use simdjson::ondemand::parser::iterate_many.

I initially tried using simdjson::ondemand::parser::iterate_many, but after a few attempts I ran into boundary-handling issues with the chunker semantics. I then referred to an earlier implementation by @rok as a reference and followed that approach.

That said, I agree that parsing JSON manually here is not ideal. I'll revisit this using iterate_many and address the other review comments as well.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Reranko05 · 2026-08-25T12:46:28Z

@pitrou I tried using simdjson::ondemand::parser::iterate_many(). Most tests pass, but PropagateErrorsNonLinewiseChunker behaves differently: malformed input is split into a separate document and the error is then reported by the JSON parser, whereas the current implementation reports a chunker error.

Manual structural parsing approach preserved the existing test behavior. @rok, since your earlier implementation was helpful here, do you have any suggestions on how to preserve the current error behavior with iterate_many()?

pitrou · 2026-08-25T12:51:17Z

Most tests pass, but PropagateErrorsNonLinewiseChunker behaves differently: malformed input is split into a separate document and the error is then reported by the JSON parser, whereas the current implementation reports a chunker error.

As long as an error is reported while reading the JSON stream, I don't think we care if it's reported by the chunker or the parser.

Reranko05 · 2026-08-26T11:33:30Z

@pitrou I gave iterate_many() more that a few attempts, but I am running into semantic differences with the previous RapidJSON implementation, especially around stopping after a complete value when it is followed by a partial or malformed value. At this point, I think manually finding the boundary of the first complete object/array and then validating that slice with simdjson may be simpler and closer to the existing behavior. Do you think that would be a reasonable approach?

pitrou · 2026-08-26T11:43:13Z

@pitrou I gave iterate_many() more that a few attempts, but I am running into semantic differences with the previous RapidJSON implementation, especially around stopping after a complete value when it is followed by a partial or malformed value.

Hmm, I see. Thanks for trying anyway :-)

At this point, I think manually finding the boundary of the first complete object/array and then validating that slice with simdjson may be simpler and closer to the existing behavior. Do you think that would be a reasonable approach?

Well, as a last resort, yes. The problem:

We're writing our own JSON parser, which means we must careful test it.
We're losing performance unless we implement our own SIMD optimizations.

Thoughts @HuaHuaY @cyb70289 ?

cyb70289 · 2026-08-27T02:47:23Z

Try to understand the issue. Is it that json strings legal for rapidjson may fail on simdjson, makes future Arrow release potentially incompatible to old version?

Writing our own optimized version looks not ideal. Can we just use simdjson? It's state-of-the-art, and even with self written object delimiter, there's still incompatibility risk I'm afraid.

Reranko05 · 2026-08-27T08:18:07Z

@cyb70289 I don't think the issue is JSON compatibility between RapidJSON and simdjson. The main issue I ran into is the boundary/streaming semantics.

The previous RapidJSON implementation uses kParseStopWhenDoneFlag, so it stops as soon as the first complete top-level value is parsed, even if the remaining input contains a partial or malformed value.

With iterate_many(), I wasn't able to reproduce that behavior: errors in subsequent/incomplete data can affect whether we successfully identify the preceding complete value.

I agree that writing our own optimized JSON parser would not be ideal. The manual approach I used, and which @rok also implemented in rok#47, only scans for the boundary of the first complete object or array while respecting strings and escapes, and then lets simdjson perform the actual JSON validation. But I agree this still introduces complexity and needs careful testing.

If there is a way to use simdjson directly while preserving the old stop-after-one-value semantics, that would definitely be preferable.

pitrou · 2026-08-27T09:06:21Z

With iterate_many(), I wasn't able to reproduce that behavior: errors in subsequent/incomplete data can affect whether we successfully identify the preceding complete value.

Is that a problem? We want to keep compatibility when parsing valid JSON streams. The failure mode for an invalid JSON stream can change.

cyb70289 · 2026-08-27T09:27:33Z

A discussion about ignoring trailing garbage in simdjson. Looks there're real use cases lenient parsing can be useful.
simdjson/simdjson#2502

pitrou · 2026-08-27T09:48:52Z

Trailing garbage is not the problem here. We are parsing a stream of valid JSON documents. We are happy to error out on trailing garbage.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copilot

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copilot

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copilot

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

pitrou · 2026-09-03T15:09:47Z

I've made enough changes that it may benefit from another person's review. @HuaHuaY Do you want to take a look?

pitrou · 2026-09-03T15:26:44Z

@github-actions crossbow submit -g cpp

github-actions · 2026-09-03T15:34:38Z

Revision: 386900c

Submitted crossbow builds: ursacomputing/crossbow @ actions-810cb09b40

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-13-cpp-amd64
test-debian-13-cpp-i386
test-debian-experimental-cpp-gcc-15
test-fedora-42-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

pitrou · 2026-09-03T16:14:42Z

(CI failures are unrelated)

Reranko05 · 2026-09-04T10:11:18Z

Thanks, @pitrou. Appreciate the additional changes and review.

Reranko05 · 2026-09-08T09:29:33Z

@HuaHuaY Could you review this when you have time? Thanks!

pitrou · 2026-09-10T09:35:58Z

@taepper Would you like to take a look at this PR?

taepper · 2026-09-10T11:38:37Z

+    if (consumed_length > 0) {
+      // If we found at least one document, also consume its trailing whitespace
+      // to avoid stray bytes at the end of the stream.
+      consumed_length += ConsumeWhitespace(input.substr(consumed_length));
+    }


Why do we care about this? We can just emit boundaries with whitespace, it is valid json

Or is the contract that consumed_length is the same as input.size() if we consume the last element of the input?

Or is the contract that consumed_length is the same as input.size() if we consume the last element of the input?

I'm not sure it's explicitly spelled out, but that would have my preference.

If I disable this code I get some test errors, though they might be harmless.

taepper · 2026-09-10T11:39:31Z

+    // XXX Should be pass a specific batch_size?
+    // The default value used by simdjson is 1MB, probably enough for most purposes.


I do not think we care about custom batch sizing

We would care in case a single JSON object is larger than the configured simdjson batch size (1MB), because it would fail parsing.

But such cases are probably rare enough that we can defer handling them to when someone reports a bug.

Ah, I misunderstood the comment. I thought this was about batch sizing for internal simdjson buffers (i.e. only for optimization purposes) and missed that this was a size limit for individual json documents

We could use batch_size equal to input.size(), if we do not want to fail in this case:

const size_t batch_size = std::max<size_t>(input.size(), 1); RETURN_NOT_OK(ToStatus(parser_.iterate_many(input, batch_size).get(stream)));

But I agree that it could be acceptable, given that the chunker for newline-delimited data does not have this limit

taepper · 2026-09-10T11:48:37Z

+    buffer_.reserve(partial.size() + block.size() + simdjson::SIMDJSON_PADDING);
+    buffer_.append(partial);
+    buffer_.append(block);


Ugh, simdjson does not have RapidJson's MultiStringStream so the current api is a rough fit. I see that call sites want this "partial", "continuation" split to consume streams efficiently

We might want to at least check for emptiness of partial. After checking call sites this seems to be the case a few times

partial being empty doesn't change the fact that we need to copy block to ensure there's enough padding.

We can rework this later by having the BoundaryFinder API take Buffer arguments instead of std::string_view, because that would let us inspect their capacity and avoid copying if the Buffer has enough padding already.

That said, chunking is already faster with this PR than it used to be with RapidJSON:

before:

ChunkJSONPrettyPrinted 755476 ns 755351 ns 932 bytes_per_second=276.193Mi/s json_size=218.757k ChunkJSONLineDelimited 94.6 ns 94.6 ns 7391992 bytes_per_second=0/s json_size=193.757k

after:

ChunkJSONPrettyPrinted 634472 ns 634465 ns 1110 bytes_per_second=388.942Mi/s json_size=258.757k ChunkJSONLineDelimited 94.5 ns 94.5 ns 7418649 bytes_per_second=0/s json_size=193.757k

(on Ubuntu 24.04 with a AMD Zen 2 CPU supporting AVX2)

I see. I agree that we will not be able to avoid at least one copy until the APIs evolve

taepper · 2026-09-10T13:07:57Z

+  // Consume the first or last JSON object (depending on `until_end`)
+  // and return the consumed JSON byte length, or 0 if no valid document
+  // can be parsed.
+  Result<size_t> ConsumeWholeObject(simdjson::padded_string_view input, bool until_end) {


I find the name until_end a little confusing

I thought that would repeat more code, such as the document stream setup and the handling of the various corner cases.

I see. I agree that duplicating the entire function is probably worse. But the outer function being called ConsumeWholeObject when it can also consume multiple objects and the inner function ConsumeDocument when normally a document contains an object, not the other way around, is misleading IMO.

Maybe the function can still be called Find (as the two functions calling it) and the boolean flag can be last

taepper · 2026-09-10T17:20:25Z

+    // XXX Hopefully this upholds for all std::string implementations
+    DCHECK_GE(buffer_.capacity() - buffer_.size(), simdjson::SIMDJSON_PADDING);


We could avoid the comment and assertion if we instead resize the buffer (instead of only calling reserve) to include padding and then construct an intermediate std::string_view{buffer_.data(), partial.size() + block.size()}

github-actions Bot added Component: C++ awaiting review Awaiting review labels Aug 21, 2026

Reranko05 added CI: Extra: C++ Run extra C++ CI and removed awaiting review Awaiting review labels Aug 21, 2026

Reranko05 marked this pull request as ready for review August 21, 2026 16:33

Reranko05 requested a review from pitrou as a code owner August 21, 2026 16:33

Copilot AI lite review requested due to automatic review settings August 21, 2026 16:33

Copilot AI reviewed Aug 21, 2026

Reranko05 requested review from kou and rok August 21, 2026 16:33

github-actions Bot added the awaiting review Awaiting review label Aug 21, 2026

Reranko05 force-pushed the gh-35460-chunker branch from e9fc9fe to be4c2a1 Compare August 24, 2026 07:52

pitrou requested changes Aug 24, 2026

View reviewed changes

Comment thread cpp/src/arrow/json/chunker.cc Outdated

Comment thread cpp/src/arrow/json/chunker.cc Outdated

Comment thread cpp/src/arrow/json/chunker.cc Outdated

Comment thread cpp/src/arrow/json/chunker.cc Outdated

Comment thread cpp/src/arrow/json/chunker.cc Outdated

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 24, 2026

Copilot AI review requested due to automatic review settings August 24, 2026 20:06

Copilot AI reviewed Aug 24, 2026

Copilot AI review requested due to automatic review settings August 27, 2026 11:31

Copilot AI reviewed Aug 27, 2026

Reranko05 added 7 commits September 3, 2026 16:54

Use ARROW_ASSIGN_OR_RAISE

a030080

Address Copilot Feedback

4348d46

Address Feedback

44d202f

Address Copilot Feedback

6d579dc

Python test fix

d737f8f

Test fixing

cfc88e2

Address Feedback

6e5ccb2

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 3, 2026

Copilot AI review requested due to automatic review settings September 3, 2026 14:56

pitrou force-pushed the gh-35460-chunker branch from 6923f56 to 008f53c Compare September 3, 2026 14:56

Copilot AI reviewed Sep 3, 2026

Copilot AI review requested due to automatic review settings September 3, 2026 14:57

pitrou force-pushed the gh-35460-chunker branch from 008f53c to 1f75592 Compare September 3, 2026 14:57

Copilot AI reviewed Sep 3, 2026

Copilot AI review requested due to automatic review settings September 3, 2026 15:03

pitrou force-pushed the gh-35460-chunker branch from 1f75592 to 8b364d1 Compare September 3, 2026 15:03

Copilot AI reviewed Sep 3, 2026

Rework code organization and buffer allocation

386900c

Copilot AI review requested due to automatic review settings September 3, 2026 15:05

Copilot AI reviewed Sep 3, 2026

taepper suggested changes Sep 10, 2026

View reviewed changes

taepper reviewed Sep 10, 2026

View reviewed changes

		// XXX Should be pass a specific batch_size?
		// The default value used by simdjson is 1MB, probably enough for most purposes.

		// XXX Hopefully this upholds for all std::string implementations
		DCHECK_GE(buffer_.capacity() - buffer_.size(), simdjson::SIMDJSON_PADDING);

Uh oh!

Conversation

Reranko05 commented Aug 21, 2026 • edited by pitrou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Reranko05 commented Aug 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reranko05 commented Aug 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reranko05 commented Aug 25, 2026

Uh oh!

pitrou commented Aug 25, 2026

Uh oh!

Reranko05 commented Aug 26, 2026

Uh oh!

pitrou commented Aug 26, 2026

Uh oh!

cyb70289 commented Aug 27, 2026

Uh oh!

Reranko05 commented Aug 27, 2026

Uh oh!

pitrou commented Aug 27, 2026

Uh oh!

cyb70289 commented Aug 27, 2026

Uh oh!

pitrou commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Sep 3, 2026

Uh oh!

pitrou commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

pitrou commented Sep 3, 2026

Uh oh!

Reranko05 commented Sep 4, 2026

Uh oh!

Reranko05 commented Sep 8, 2026

Uh oh!

pitrou commented Sep 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Reranko05 commented Aug 21, 2026 •

edited by pitrou

Loading

taepper Sep 10, 2026 •

edited

Loading

taepper Sep 10, 2026 •

edited

Loading

taepper Sep 10, 2026 •

edited

Loading