Uh oh!
There was an error while loading. Please reload this page.
GH-50567: [C++] Introduce JsonWriter and migrate integration JSON writer - #50568
Conversation
fd69d50 to
bb7fd90CompareReranko05
commented
Jul 21, 2026
@kou could you review this PR when you have a chance? |
kou
left a comment
There was a problem hiding this comment.
Could you check performance difference?
Uh oh!
There was an error while loading. Please reload this page.
Reranko05
commented
Jul 21, 2026
@kou, I compared the serialization performance of main: 2157.4 ms I didn't observe any measurable performance regression. |
Reranko05
commented
Jul 21, 2026
@kou I investigated the failing workflows and they appear to be unrelated to this PR. Could you please verify? |
bb7fd90 to
c983eb0CompareThere was a problem hiding this comment.
Thanks for working on this @Reranko05! Exciting to see this work!
We should probably add meson support for json_writer, see (cpp/src/arrow/meson.build, cpp/src/arrow/json/meson.build, cpp/src/arrow/integration/meson.build).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rok
commented
Jul 21, 2026
Another thought - do we have conbench cpp micro benchmarks to monitor improvement of json parsing and writing? |
There was a problem hiding this comment.
Pull request overview
This PR adds a reusable arrow::json::JsonWriter wrapper around simdjson’s builder API and migrates the C++ integration JSON writer (and its tests) away from RapidJSON’s writer, as part of the incremental RapidJSON → simdjson migration.
Changes:
- Added
arrow::json::JsonWriter(header/impl) plus unit tests. - Migrated integration JSON serialization code paths to emit JSON using
JsonWriter. - Updated CMake/test linkage and CI environment to ensure simdjson is available/linked where needed.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/arrow/json/json_writer.h | Introduces the JsonWriter public API. |
| cpp/src/arrow/json/json_writer.cc | Implements JsonWriter on top of simdjson builder primitives. |
| cpp/src/arrow/json/json_writer_test.cc | Adds unit coverage for basic writer operations. |
| cpp/src/arrow/json/CMakeLists.txt | Adds the new unit test and links simdjson for the test target. |
| cpp/src/arrow/integration/json_internal.h | Updates writer function signatures to accept arrow::json::JsonWriter*. |
| cpp/src/arrow/integration/json_internal.cc | Migrates integration JSON emission to JsonWriter (writer-side). |
| cpp/src/arrow/integration/json_integration.cc | Switches IntegrationJsonWriter implementation to JsonWriter. |
| cpp/src/arrow/integration/json_integration_test.cc | Updates integration tests to use JsonWriter for schema/array JSON generation. |
| cpp/src/arrow/integration/CMakeLists.txt | Links simdjson where integration tests/executables now depend on it. |
| cpp/src/arrow/CMakeLists.txt | Adds json/json_writer.cc to Arrow JSON sources and links simdjson for integration targets. |
| ci/docker/ubuntu-24.04-cpp.dockerfile | Sets simdjson_SOURCE=BUNDLED to ensure simdjson availability in CI images. |
Uh oh!
There was an error while loading. Please reload this page.
| ArrayWriter(const std::string& name, const Array& array, JsonWriter* writer) | ||
| : name_(name), array_(array), writer_(writer) {} |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Reranko05
commented
Jul 21, 2026
I wasn't able to find any existing Conbench C++ microbenchmarks covering JSON parsing or writing. I did run a local benchmark comparing |
a25a245 to
106ee16Comparerok
commented
Jul 21, 2026
It then seems we currently don't benchmark serializing and deserializing json. It would be useful to introduce some, but perhaps that is out of scope for this ticket. |
979ca51 to
211c95cComparedd041cd to
0b5a345Compare0b5a345 to
7a708b0Compare7a708b0 to
29ada49Compare@pitrou@rok@kou I ran into a As a debugging experiment, I qualified them as Since my PR doesn't modify |
Reranko05
commented
Jul 28, 2026
Except the comment above, I think rest of the CI failures are unrelated to this PR. |
pitrou
commented
Jul 28, 2026
Yes, it's quite typical of unity builds which amalgamate several source files ( |
Reranko05
commented
Jul 28, 2026
@pitrou Thanks for confirming! Using that approach fixed the CI issue. |
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6c2c8c7. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
What changes are included?
This PR introduces a reusable
JsonWriterwrapper around simdjson's JSON builder API and migrates the integration JSON writer to use it instead of RapidJSON.Specifically, this PR:
JsonWriterabstraction inarrow/json.JsonWriter.JsonWriter.JsonWriter.This is part of the incremental migration from RapidJSON to simdjson.
Performance
I compared the serialization performance of
IntegrationJsonWriter::WriteRecordBatch()+Finish()using a temporary benchmark with a 1M-rowRecordBatch. The benchmark was run 10 times on bothmainand this branch.mainNo measurable performance regression was observed.
Are these changes tested?
Yes.
I added unit tests for
JsonWriterand verified that both JSON and integration tests pass locally:arrow-json-testarrow-json-integration-testAre there any user-facing changes?
No.