Repository navigation
Conversation
Reranko05
changed the title
3842058 to
9813fc2
Compare
Reranko05
changed the title
|
Will mark this PR |
| // * arrow::TableBatchReader and Arrow iterators for converting Arrow tables back | ||
| // into row-based JSON data | ||
|
|
||
| namespace arrow { |
There was a problem hiding this comment.
Example code shouldn't be in the arrow namespace because it showcases how third-party code using Arrow C++ could look like.
There was a problem hiding this comment.
Ah, okay. I got it.
| // * arrow::internal::JsonWriter for writing JSON values | ||
| // * arrow::internal::ParseJsonObject and related helpers for parsing JSON |
There was a problem hiding this comment.
Examples should not use any internal Arrow APIs.
There was a problem hiding this comment.
Okay, understood.
github-actions
Bot
added
awaiting committer review
|
I have not got the time to review this myself after changing it into plain simdjson. I will mark it as |
| // Parse each row and append its values to the corresponding Arrow builders. | ||
| for (const auto& json : rows) { | ||
| ARROW_ASSIGN_OR_RAISE(auto element, | ||
| Unwrap(parser.parse(json), "Failed to parse JSON row")); | ||
| ARROW_ASSIGN_OR_RAISE(auto object, | ||
| Unwrap(element.get_object(), "Expected a JSON object")); | ||
|
|
||
| for (int i = 0; i < schema->num_fields(); ++i) { | ||
| const auto& field = schema->field(i); | ||
| arrow::ArrayBuilder* builder = batch_builder->GetField(i); | ||
|
|
||
| ARROW_ASSIGN_OR_RAISE(auto value, GetField(object, field->name())); | ||
|
|
||
| if (!value.has_value()) { | ||
| ARROW_RETURN_NOT_OK(builder->AppendNull()); | ||
| } else { | ||
| ARROW_RETURN_NOT_OK(AppendJsonValue(*value, field->type(), builder)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we keep column based iteration (not row based iteration) like the RapidJSON version so that we demonstrate how to use a visitor to convert row based data to Arrow data?
There was a problem hiding this comment.
I changed the impl to do column based iteration.
| for (int64_t i = 0; i < batch->num_rows(); ++i) { | ||
| ARROW_ASSIGN_OR_RAISE(auto row, ConvertRowToJson(*batch, i)); | ||
| rows.push_back(std::make_shared<std::string>(std::move(row))); | ||
| } |
There was a problem hiding this comment.
Can we keep column based iteration (not row based iteration) like the RapidJSON version so that we demonstrate how to use a visitor to convert Arrow data to row based data?
@kou @pitrou @taepper I am a bit confused about whether this failure is related to my PR, since another PR passes the same CI job even after I rebased mine. Could someone clarify? |
|
BTW, I have not yet updated the |
|
Hi, yes, I can take a look at this Can you quickly confirm my understanding re/ the commit history because I am a little bit confused (and the history has since been rebased & force-pushed) The initial commit
Yes, I would also think that your change to |
@taepper I initially used internal Arrow APIs to write the example code, then I got to know that I should write it in plain simdjson and then it was that my impl was row based but we needed column based iteration, so yeah they were considerable feedback changes 😅 |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Could you update the documents and the PR description?
github-actions
Bot
added
awaiting changes
taepper
left a comment
taepper
left a comment
There was a problem hiding this comment.
(I actually found the row-based conversion to arrow more readable [and it would also allow usage of the simdjson's ondemand backend]. The visit on arrow arrays can still be demonstrated for the back-conversion to json in JsonColumnWriter.)
Splitting the two conversions json->arrow and arrow->json into two separate files would make this more well-organized? I would also imagine any user that wants to follow this code example in their own project should organise it in separate files as well
| ARROW_RETURN_NOT_OK( | ||
| AppendJsonValue(*child, field->type(), builder->child_builder(i).get())); | ||
| // A JSON value, or std::nullopt if it is missing. | ||
| using JsonValue = std::optional<simdjson::dom::element>; |
There was a problem hiding this comment.
Ugh, do we really need this special type? Can't we append a simdjson null document in those cases instead?
Also, the code comment below mentions:
Missing fields, and fields of null objects, are returned as std::nullopt
but I do not think this is true? The way I understand it is that fields of null objects are returned as null, but in case of missing fields this:
ARROW_ASSIGN_OR_RAISE(auto object,
Unwrap(value->get_object(), "Failed to get JSON object"));
ARROW_ASSIGN_OR_RAISE(auto child, GetField(object, name));
will raise an error instead?
Overall, I think avoiding this special type could make the example easier to understand
There was a problem hiding this comment.
I tested with a missing metrics field, and conversion succeeds, producing an Arrow null without errors. I think using a simdjson null value instead might require more code than the current std::optional approach. Could you explain how you'd implement this using the DOM API?
github-actions
Bot
added
awaiting change review
@kou Addressed feedbacks and updated docs and PR description. |
I am fine with any approach. @kou @pitrou what do you think?
Same as above |
a9f6790 to
a5d38cd
Compare
|
I am not an Arrow member, so feel free to disregard my point-of-view. But in my opinion, for the example to be reusable by other code it should be a direct conversion to arrow structures, instead of doing a row-wise buffering into Additionally, the custom Note that the actual conversion code got a lot briefer. Feel free to cherry pick (parts of) the changes. Yet again, I do not want to block this PR unnecessarily, so if other reviewers feel differently, feel free to disregard |
|
I checked the history. It seems that we wanted to show an example of how to convert row-based data to Apache Arrow data efficiently:
If simdjson isn't suitable for column-based conversion, we may keep using RapidJSON in this example. In that case, I want to stop bundling RapidJSON and use only system RapidJSON. If simdjson is suitable for row-based conversion and it's fast, we may add a new example that shows fast row-based conversion. If row-based conversion is always faster than column-based conversion in any approach, we can remove the column-based conversion example. Otherwise, I think that we should keep the column-based conversion example. |
|
I tested the 3 different implementations (RapidJSON, column-wise simdjson, row-wise simdjson): Median wall time (Ryzen 7 PRO 8700GE, GCC 13, Arrow release flags, single thread).
|
|
This advantage still holds for larger datasets:
Also note peak memory reduced from 26 GB to 19 GB with the row-wise version |
|
OK. Let's add an example that uses row-based approach with simdjson. Let's revisit the current column-based approach example as a separated task. We may need another use case such as database not JSON for column-based approach. Or we may remove column-based approach entirely if row-based approach is better than column-based approach for non JSON use cases too. |
Rationale for this change
This PR continues the simdjson migration by replacing RapidJSON usage in the C++ example demonstrating row-to-columnar and columnar-to-row conversion using simdjson and Apache Arrow.
Changes
simdjson_row_converter.ccfor JSON parsing, Arrow array construction, and JSON serialization.rapidjson_row_converter.cc.Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: