Repository navigation
Conversation
3842058 to
9813fc2
Compare
|
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.
|
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?
9519b33 to
3673c4d
Compare
@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
There was a problem hiding this comment.
Could you update the documents and the PR description?
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?
@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
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: