You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Reranko05 (via GitHub)" <gi...@apache.org> on 2026/08/26 22:58:37 UTC
[PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Reranko05 opened a new pull request, #51014:
URL: https://github.com/apache/arrow/pull/51014
### Rationale for this change
This PR continues the simdjson migration by replacing RapidJSON usage in the C++ JSON test and testing utilities with simdjson and Arrow's existing `JsonWriter`.
### Changes
- Replace `rapidjson::Writer` and `StringBuffer` usage in JSON test utilities with `JsonWriter`.
- Replace RapidJSON-based JSON pretty-printing with simdjson-based parsing and serialization.
- Use `JsonWriter` to correctly escape JSON object keys during pretty-printing.
- Replace RapidJSON DOM parsing in `TensorFromJSON()` with simdjson's DOM API.
- Add regression coverage for escaped JSON object keys in `PrettyPrint()`.
- Add test coverage for `TensorFromJSON()` with strides and dimension names.
- Update test linking to include `arrow::simdjson`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3950245984
##########
cpp/src/arrow/json/reader_test.cc:
##########
@@ -550,10 +551,14 @@ class StreamingReaderTestBase {
auto options = GenerateOptions::Defaults();
options.null_probability = 0;
for (int i = 0; i < num_rows; ++i) {
- StringBuffer string_buffer;
- Writer writer(string_buffer);
+ Writer writer;
ABORT_NOT_OK(Generate(data_fields, engine, &writer, options));
- std::string json = string_buffer.GetString();
+
+ auto json_result = writer.GetString();
+ ABORT_NOT_OK(json_result.status());
+ auto json_view = std::move(json_result).ValueOrDie();
+ std::string json(json_view);
Review Comment:
Thanks! `GetString()` currently returns `Result<std::string_view>`, so `writer.GetString().As<std::string>()` isn't available in this API. I used `std::string(writer.GetString().ValueOrDie())` instead, which explicitly constructs a `std::string` from the returned` std::string_view`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3950355207
##########
cpp/src/arrow/util/simdjson_internal.h:
##########
@@ -27,6 +27,7 @@
#include <simdjson.h>
+#include "arrow/json/json_writer_internal.h"
Review Comment:
Removed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951241155
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,17 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
Well, the `PrettyPrint` function lives in `arrow/json/test_common.h` so we should keep the test in this directory too.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951432009
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,17 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
Yes, sorry, I meant just that!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951550237
##########
cpp/src/arrow/json/test_common.h:
##########
@@ -258,14 +261,26 @@ inline static Status ParseFromString(ParseOptions options, string_view src_str,
}
static inline std::string PrettyPrint(string_view one_line) {
- rj::Document document;
+ simdjson::ondemand::parser parser;
// Must pass size to avoid ASAN issues.
- document.Parse(one_line.data(), one_line.size());
- rj::StringBuffer sb;
- rj::PrettyWriter<rj::StringBuffer> writer(sb);
- document.Accept(writer);
- return sb.GetString();
+ simdjson::padded_string json(one_line.data(), one_line.size());
Review Comment:
I moved the regression test to `simdjson_internal_test.cc` and it now tests `PrettyPrintJsonValue()` directly.
I also checked the remaining uses of the `PrettyPrint(std::string_view)` helper: it is only used by `parser_benchmark.cc`. So if I replace it with `fractured_json_string()`, the existing test suite won't exercise that helper anymore.
Given that `PrettyPrintJsonValue()` would then have no caller other than its own test, do you intend for us to keep `PrettyPrintJsonValue()` as a standalone utility, or should `PrettyPrint()` simply use `fractured_json_string()` and we remove the custom formatter?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951830873
##########
cpp/src/arrow/json/test_common.h:
##########
@@ -258,14 +261,26 @@ inline static Status ParseFromString(ParseOptions options, string_view src_str,
}
static inline std::string PrettyPrint(string_view one_line) {
- rj::Document document;
+ simdjson::ondemand::parser parser;
// Must pass size to avoid ASAN issues.
- document.Parse(one_line.data(), one_line.size());
- rj::StringBuffer sb;
- rj::PrettyWriter<rj::StringBuffer> writer(sb);
- document.Accept(writer);
- return sb.GetString();
+ simdjson::padded_string json(one_line.data(), one_line.size());
Review Comment:
> or should `PrettyPrint()` simply use `fractured_json_string()` and we remove the custom formatter?
If `fractured_json_string` is good enough for our use case(s), then let's do just that ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3950233365
##########
cpp/src/arrow/json/reader_test.cc:
##########
@@ -550,10 +551,14 @@ class StreamingReaderTestBase {
auto options = GenerateOptions::Defaults();
options.null_probability = 0;
for (int i = 0; i < num_rows; ++i) {
- StringBuffer string_buffer;
- Writer writer(string_buffer);
+ Writer writer;
ABORT_NOT_OK(Generate(data_fields, engine, &writer, options));
- std::string json = string_buffer.GetString();
+
+ auto json_result = writer.GetString();
+ ABORT_NOT_OK(json_result.status());
+ auto json_view = std::move(json_result).ValueOrDie();
+ std::string json(json_view);
Review Comment:
Thanks! `GetString()` currently returns `Result<std::string_view>`, so `writer.GetString().As<std::string>()` isn't available in this API. I used `std::string(writer.GetString().ValueOrDie())` instead, which explicitly constructs a `std::string` from the returned` std::string_view`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51014:
URL: https://github.com/apache/arrow/pull/51014#issuecomment-5582071663
Revision: 259feafa9bdc843d1c25040e2521486669b06694
Submitted crossbow builds: [ursacomputing/crossbow @ actions-f8a3b2cdad](https://github.com/ursacomputing/crossbow/branches/all?query=actions-f8a3b2cdad)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/34206430261/job/101996882646)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/34206431993/job/101996888599)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/34206431386/job/101996887218)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/34206431745/job/101996887532)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/34206432406/job/101996889637)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/34206431822/job/101996888047)|
|test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/34206429046/job/101996878698)|
|test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/34206429952/job/101996881524)|
|test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/34206429012/job/101996878378)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/34206430697/job/101996884041)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/34206430217/job/101996882147)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/34206429299/job/101996879256)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/34206428526/job/101996876931)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/34206432706/job/101996890735)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/34206432138/job/101996889363)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/34206427999/job/101996875391)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/34206428233/job/101996876499)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/34206428776/job/101996877940)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/34206427335/job/101996873455)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/34206429648/job/101996880837)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on PR #51014:
URL: https://github.com/apache/arrow/pull/51014#issuecomment-5572415239
@pitrou Could you review this when you have time. (CI failures seem unrelated)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3871670890
##########
cpp/src/arrow/util/simdjson_internal.h:
##########
@@ -294,6 +295,118 @@ Status VisitJsonValue(simdjson::ondemand::value value, ObjectFn&& object_fn,
return Status::Invalid("Unreachable");
}
+inline Status PrettyPrintJsonValue(simdjson::ondemand::value value, std::string* out,
Review Comment:
Can we please move this to `simdjson_internal.cc`? There's no reason to inline the implement in the `.h` file.
Also, can this return `Result<std::string>`? It would be more idiomatic than an out-pointer parameter.
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,17 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
This doesn't seem like a great place for this test but I can't think of anything better.
##########
cpp/src/arrow/json/reader_test.cc:
##########
@@ -550,10 +551,14 @@ class StreamingReaderTestBase {
auto options = GenerateOptions::Defaults();
options.null_probability = 0;
for (int i = 0; i < num_rows; ++i) {
- StringBuffer string_buffer;
- Writer writer(string_buffer);
+ Writer writer;
ABORT_NOT_OK(Generate(data_fields, engine, &writer, options));
- std::string json = string_buffer.GetString();
+
+ auto json_result = writer.GetString();
+ ABORT_NOT_OK(json_result.status());
+ auto json_view = std::move(json_result).ValueOrDie();
+ std::string json(json_view);
Review Comment:
I think you can write this more concisely as:
```suggestion
std::string json = writer.GetString().As<std::string>();
```
##########
cpp/src/arrow/util/simdjson_internal.h:
##########
@@ -27,6 +27,7 @@
#include <simdjson.h>
+#include "arrow/json/json_writer_internal.h"
Review Comment:
We shouldn't be including `arrow/json` IMHO. Let's wait for https://github.com/apache/arrow/pull/50990 to be merged perhaps?
##########
cpp/src/arrow/json/test_common.h:
##########
@@ -258,14 +261,26 @@ inline static Status ParseFromString(ParseOptions options, string_view src_str,
}
static inline std::string PrettyPrint(string_view one_line) {
- rj::Document document;
+ simdjson::ondemand::parser parser;
// Must pass size to avoid ASAN issues.
- document.Parse(one_line.data(), one_line.size());
- rj::StringBuffer sb;
- rj::PrettyWriter<rj::StringBuffer> writer(sb);
- document.Accept(writer);
- return sb.GetString();
+ simdjson::padded_string json(one_line.data(), one_line.size());
Review Comment:
Why not use [`fractured_json_string`](https://simdjson.github.io/simdjson/namespacesimdjson.html#a64861b6d9d98dc3c85683b50cc6d224a)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3956005951
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,12 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
I elected to remove this test since `PrettyPrint` is now a trivial wrapper.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951230347
##########
cpp/src/arrow/json/reader_test.cc:
##########
@@ -550,10 +551,14 @@ class StreamingReaderTestBase {
auto options = GenerateOptions::Defaults();
options.null_probability = 0;
for (int i = 0; i < num_rows; ++i) {
- StringBuffer string_buffer;
- Writer writer(string_buffer);
+ Writer writer;
ABORT_NOT_OK(Generate(data_fields, engine, &writer, options));
- std::string json = string_buffer.GetString();
+
+ auto json_result = writer.GetString();
+ ABORT_NOT_OK(json_result.status());
+ auto json_view = std::move(json_result).ValueOrDie();
+ std::string json(json_view);
Review Comment:
Oh, you're right. I meant `writer.GetString().As<std::string>().ValueOrDie()` but your solution is more concise.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on PR #51014:
URL: https://github.com/apache/arrow/pull/51014#issuecomment-5582080758
@pitrou Thanks for the review!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #51014:
URL: https://github.com/apache/arrow/pull/51014#issuecomment-5582796141
CI failures are unrelated, I'll merge.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3950345234
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,17 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
I agree. Would it be better to move this test to `simdjson_internal_test.cc`, since it specifically covers the JSON pretty-printing utility?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951214255
##########
cpp/src/arrow/json/test_common.h:
##########
@@ -258,14 +261,26 @@ inline static Status ParseFromString(ParseOptions options, string_view src_str,
}
static inline std::string PrettyPrint(string_view one_line) {
- rj::Document document;
+ simdjson::ondemand::parser parser;
// Must pass size to avoid ASAN issues.
- document.Parse(one_line.data(), one_line.size());
- rj::StringBuffer sb;
- rj::PrettyWriter<rj::StringBuffer> writer(sb);
- document.Accept(writer);
- return sb.GetString();
+ simdjson::padded_string json(one_line.data(), one_line.size());
Review Comment:
We required simdjson 4.3.0 and can use the bundled version for older OSes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on PR #51014:
URL: https://github.com/apache/arrow/pull/51014#issuecomment-5579425097
@pitrou, I have made the dicussed changes. (CI failures doesnt seem related)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951248081
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,17 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
Actually, we can make the test call the new `PrettyPrintJsonValue`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951220704
##########
cpp/src/arrow/json/test_common.h:
##########
@@ -258,14 +261,26 @@ inline static Status ParseFromString(ParseOptions options, string_view src_str,
}
static inline std::string PrettyPrint(string_view one_line) {
- rj::Document document;
+ simdjson::ondemand::parser parser;
// Must pass size to avoid ASAN issues.
- document.Parse(one_line.data(), one_line.size());
- rj::StringBuffer sb;
- rj::PrettyWriter<rj::StringBuffer> writer(sb);
- document.Accept(writer);
- return sb.GetString();
+ simdjson::padded_string json(one_line.data(), one_line.size());
Review Comment:
Understood.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #51014:
URL: https://github.com/apache/arrow/pull/51014
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3956017818
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,12 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
Agreed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #51014:
URL: https://github.com/apache/arrow/pull/51014#issuecomment-5581997355
@github-actions crossbow submit -g cpp
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3888703862
##########
cpp/src/arrow/util/simdjson_internal.h:
##########
@@ -27,6 +27,7 @@
#include <simdjson.h>
+#include "arrow/json/json_writer_internal.h"
Review Comment:
Okay will pause this until #50990 is merged.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3950349700
##########
cpp/src/arrow/json/test_common.h:
##########
@@ -258,14 +261,26 @@ inline static Status ParseFromString(ParseOptions options, string_view src_str,
}
static inline std::string PrettyPrint(string_view one_line) {
- rj::Document document;
+ simdjson::ondemand::parser parser;
// Must pass size to avoid ASAN issues.
- document.Parse(one_line.data(), one_line.size());
- rj::StringBuffer sb;
- rj::PrettyWriter<rj::StringBuffer> writer(sb);
- document.Accept(writer);
- return sb.GetString();
+ simdjson::padded_string json(one_line.data(), one_line.size());
Review Comment:
Look https://github.com/apache/arrow/pull/51142#issuecomment-5571581385
##########
cpp/src/arrow/util/simdjson_internal.h:
##########
@@ -294,6 +295,118 @@ Status VisitJsonValue(simdjson::ondemand::value value, ObjectFn&& object_fn,
return Status::Invalid("Unreachable");
}
+inline Status PrettyPrintJsonValue(simdjson::ondemand::value value, std::string* out,
Review Comment:
Moved.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3951370774
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -321,5 +321,17 @@ TEST(BlockParser, AdHoc) {
R"([{"c":true, "d": "1991-02-03"}, {"c":false, "d":"2019-04-01"}])"});
}
+TEST(JsonTest, PrettyPrintEscapesObjectKeys) {
Review Comment:
Just to confirm, since `PrettyPrintJsonValue` is now in `simdjson_internal.{h,cc}`, do you mean moving this test to `simdjson_internal_test.cc` and testing `PrettyPrintJsonValue` directly?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3954162835
##########
cpp/src/arrow/json/test_common.h:
##########
@@ -258,14 +261,26 @@ inline static Status ParseFromString(ParseOptions options, string_view src_str,
}
static inline std::string PrettyPrint(string_view one_line) {
- rj::Document document;
+ simdjson::ondemand::parser parser;
// Must pass size to avoid ASAN issues.
- document.Parse(one_line.data(), one_line.size());
- rj::StringBuffer sb;
- rj::PrettyWriter<rj::StringBuffer> writer(sb);
- document.Accept(writer);
- return sb.GetString();
+ simdjson::padded_string json(one_line.data(), one_line.size());
Review Comment:
I switched `PrettyPrint()` to use `simdjson::fractured_json_string()` directly and removed the custom `PrettyPrintJsonValue()` formatter.
I verified the relevant JSON tests:
- `ChunkerTest.PrettyPrinted` — passed
- `ChunkerTest.StraddlingPrettyPrinted` — passed
- `JsonTest.PrettyPrintEscapesObjectKeys` — updated the expected output from the previous 4-space multiline RapidJSON formatting:
```cpp
{
"a"b": 1,
"a\b": 2
}
```
to FracturedJson's compact formatting:
```cpp
{ "a"b": 1, "a\b": 2 }
```
and the test passed.
The escaped-key behavior is still covered by the test, while the custom recursive pretty-printing implementation and its dedicated test have been removed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51013: [C++] Replace RapidJSON in JSON test utilities [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #51014:
URL: https://github.com/apache/arrow/pull/51014#discussion_r3950233365
##########
cpp/src/arrow/json/reader_test.cc:
##########
@@ -550,10 +551,14 @@ class StreamingReaderTestBase {
auto options = GenerateOptions::Defaults();
options.null_probability = 0;
for (int i = 0; i < num_rows; ++i) {
- StringBuffer string_buffer;
- Writer writer(string_buffer);
+ Writer writer;
ABORT_NOT_OK(Generate(data_fields, engine, &writer, options));
- std::string json = string_buffer.GetString();
+
+ auto json_result = writer.GetString();
+ ABORT_NOT_OK(json_result.status());
+ auto json_view = std::move(json_result).ValueOrDie();
+ std::string json(json_view);
Review Comment:
Thanks! `GetString()` currently returns `Result<std::string_view>`, so `writer.GetString().As<std::string>()` isn't available in this API. I used `std::string(writer.GetString().ValueOrDie())` instead, which explicitly constructs a `std::string` from the returned` std::string_view`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org