Repository navigation
Conversation
|
Our internal integration crashed due to this UB. I've proposed a fix, please take a look when you have time. Thanks! @kou @pitrou @Reranko05 |
|
@rok I saw you have some relevant commits. Could you help review this? |
| ARROW_ASSIGN_OR_RAISE( | ||
| auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), | ||
| "Failed to determine JSON type")); | ||
| if (json_type == sj::json_type::number) { | ||
| ARROW_ASSIGN_OR_RAISE(auto int_value, internal::GetJsonAs<int64_t>(json_obj)); | ||
| return builder_->Append(int_value != 0); | ||
| } | ||
| if (json_type != sj::json_type::boolean) { | ||
| return Status::Invalid("Expected JSON boolean for type ", type_->ToString()); | ||
| } |
There was a problem hiding this comment.
This logic seems to repeat at least partially two more times below. Could we abstract it into the helper as well? Not at any cost of course.
There was a problem hiding this comment.
Agreed. Added a GetJsonType helper to save some typing.
|
@wgtmac looks good, I'd DRY it a bit if possible, not other thoughts. |
|
I am a bit preoccupied with my university exams right now. I will take a look at this tomorrow! |
|
@Reranko05 goog luck with the exams, please ignore this until they are past. |
Rationale for this change
Fix UB in
StructConverter::AppendValue(), which reused a simdjson ondemand value after get(array) failed.See #55497.
What changes are included in this PR?
Are these changes tested?
Added a case matching the nested struct/object notation from the issue.
Are there any user-facing changes?
No.
Was AI used for this PR?
PR code and description written by:
Reviewed before submission by: