GH-50779: [C++][Parquet] Replace remaining RapidJSON usage with simdjson - #50781
GH-50779: [C++][Parquet] Replace remaining RapidJSON usage with simdjson#50781Reranko05 wants to merge 4 commits into
Conversation
|
|
d026d1f to
0230946
Compare
|
I'm migrating
With RapidJSON this was straightforward because the DOM is reusable, but with Is the expected approach here to restructure the code into a single pass, or is there another way to serialize an already-inspected Thanks! |
|
As per docs here you could probably use the // Inspect crs_object...
// If it was not a recognized lon/lat CRS:
ARROW_ASSIGN_OR_RAISE(
auto ignored,
::arrow::internal::GetSimdjsonResult(
crs_object.reset(), "Failed to reset 'crs' object: "));
ARROW_ASSIGN_OR_RAISE(
auto raw_crs,
::arrow::internal::GetSimdjsonResult(
crs_object.raw_json(), "Failed to get raw 'crs' JSON: "));
return std::string(raw_crs);Check also |
|
Thanks @rok, However, one test still fails because Would you prefer keeping the previous compact formatting or preserving the original JSON formatting? |
|
@Reranko05 let's avoid behavior changes. Perhaps we can use simdjson::minify? |
270af67 to
777999e
Compare
|
I also see many RapidJSON references in cpp/cmake_modules/ThirdpartyToolchain.cmake. Are we ready to remove those yet? (I don't know, hence the question) |
| json << R"(, "crs": )" << buffer.GetString(); | ||
| ::arrow::json::JsonWriter writer; | ||
| writer.String(crs); | ||
| json << R"(, "crs": )" << writer.GetString().ValueUnsafe(); |
There was a problem hiding this comment.
Can we use JsonWriter for all JSON build instead of mixing manual JSON build and JsonWriter build?
There was a problem hiding this comment.
Good point. That would require refactoring the entire ToJSON() implementation instead of only the crs field. I wasn't sure if that was in scope for this PR. Should I include that refactoring here?
@rok RapidJSON is no longer used by Parquet after this change, so I've removed the Parquet-specific dependency. However, there are still many RapidJSON users elsewhere in Arrow (e.g. |
|
|
||
| auto padded_json = simdjson::padded_string(json_string); | ||
|
|
||
| if (auto error = parser.iterate(padded_json).get(document)) { |
There was a problem hiding this comment.
CheckJsonValid() no longer validates the full JSON output: it creates an On-Demand document but never consumes it, so only stage-1 errors are detected. This can let malformed printer output pass the test. The previous parser also intentionally enabled full precision and NaN/Inf, so the replacement should preserve that contract while actually traversing and validating the document.
There was a problem hiding this comment.
The NaN/Inf case is a very unfortunate one. While not being part of the official JSON RFC, it will be added to the next simdjson release: simdjson/simdjson#2696
I added behavior preserving json parsing to our testing helpers in from_string.cc:
Maybe that work-around is also acceptable here?
There was a problem hiding this comment.
(in the case we really want to keep supporting NaN/Inf here, the tests were using json to generate test data for our Float computations so it was required. Again, it is not part of the JSON RFC so dropping support for it here is defendable, although many JSON libraries such as RapidJSON do support it)
There was a problem hiding this comment.
I did not preserve the previous NaN/Inf behavior, since simdjson doesn't currently support it. Should we preserve the previous NaN/Inf behavior here?
|
I've posted several review comments (some are posted by Codex as I'm not that familiar with simdjson). Let me know what you think. |
487d90f to
742921a
Compare
742921a to
d57370a
Compare
d57370a to
38042c2
Compare
38042c2 to
f0953e2
Compare
f0953e2 to
57bf182
Compare
57bf182 to
32d9dac
Compare
32d9dac to
4121d9a
Compare
4121d9a to
0fcef4d
Compare
Rationale for this change
This PR continues the simdjson migration by replacing the remaining RapidJSON usages under
cpp/src/parquetwith simdjson andJsonWriter. It also removes the remaining unnecessary RapidJSON dependencies from the Parquet test target and Meson build configuration.What changes are included in this PR?
reader_test.ccwith simdjson.geospatial/util_json_internal.ccwith simdjson andJsonWriter.types.ccwithJsonWriter.rapidjson_depdependency fromcpp/src/parquet/meson.build.CMakeLists.txt.