bkietz commented on code in PR #36018:
URL: https://github.com/apache/arrow/pull/36018#discussion_r1254451565
##########
cpp/src/arrow/array/data.cc:
##########
@@ -384,26 +382,31 @@ void ArraySpan::FillFromScalar(const Scalar& value) {
this->child_data[i].FillFromScalar(*scalar.value[i]);
}
} else if (is_union(type_id)) {
+ // Dense union needs scratch space to store both offsets and a type code
+ struct UnionScratchSpace {
+ alignas(int64_t) uint8_t type_code;
+ alignas(int64_t) uint8_t offsets[sizeof(int32_t) * 2];
+ };
+ static_assert(sizeof(UnionScratchSpace) <=
sizeof(UnionScalar::scratch_space_));
+ auto* union_scratch_space =
+ new (checked_cast<const UnionScalar&>(value).scratch_space_)
UnionScratchSpace{};
Review Comment:
It is a trivial type but those still have lifetimes, and accessing objects
outside their lifetime is UB. That's probably excessively pedantic though; the
c++20 addition of [implicit
lifetime](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0593r6.html#when-to-create-objects)
was mostly a formal acceptance of cases where we just cast the storage pointer
to the object pointer and start accessing.
TL;DR: if you prefer
```suggestion
auto* union_scratch_space =
reinterpret_cast<UnionScratchSpace*>(checked_cast<const
UnionScalar&>(value).scratch_space_);
```
that should be fine
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
