bkietz commented on code in PR #41421:
URL: https://github.com/apache/arrow/pull/41421#discussion_r1585103195
##########
cpp/src/arrow/scalar.h:
##########
@@ -278,20 +283,32 @@ struct ARROW_EXPORT BaseBinaryScalar : public
internal::PrimitiveScalarBase {
struct ARROW_EXPORT BinaryScalar
: public BaseBinaryScalar,
private internal::ArraySpanFillFromScalarScratchSpace<BinaryScalar> {
- using BaseBinaryScalar::BaseBinaryScalar;
using TypeClass = BinaryType;
using ArraySpanFillFromScalarScratchSpace =
internal::ArraySpanFillFromScalarScratchSpace<BinaryScalar>;
+ explicit BinaryScalar(std::shared_ptr<DataType> type)
+ : BaseBinaryScalar(std::move(type)),
+ ArraySpanFillFromScalarScratchSpace(this->value) {}
+
+ BinaryScalar(std::shared_ptr<Buffer> value, std::shared_ptr<DataType> type)
+ : BaseBinaryScalar(std::move(value), std::move(type)),
+ ArraySpanFillFromScalarScratchSpace(this->value) {}
+
+ BinaryScalar(std::string s, std::shared_ptr<DataType> type)
+ : BaseBinaryScalar(std::move(s), std::move(type)),
+ ArraySpanFillFromScalarScratchSpace(this->value) {}
Review Comment:
FWIW, from my read I think this *was* actually UB because base class
constructors run first. For example in the case of BinaryScalar, the
constructor for `ArraySpanFillFromScalarScratchSpace<BinaryScalar>` ran first
and downcast itself to a `BinaryScalar` which hadn't been constructed yet. The
way you have it written now, `BaseBinaryScalar` will be constructed (including
its `value`) before `ArraySpanFillFromScalarScratchSpace` references it.
##########
cpp/src/arrow/scalar.cc:
##########
@@ -705,7 +713,9 @@ Result<std::shared_ptr<Scalar>>
StructScalar::field(FieldRef ref) const {
RunEndEncodedScalar::RunEndEncodedScalar(std::shared_ptr<Scalar> value,
std::shared_ptr<DataType> type)
- : Scalar{std::move(type), value->is_valid}, value{std::move(value)} {
+ : Scalar{std::move(type), value->is_valid},
+ ArraySpanFillFromScalarScratchSpace(run_end_type()->id()),
Review Comment:
Since this is implicitly `((RunEndEncodedScalar*)this)->run_end_type()` it
will probably trigger the same issue eventually.
```suggestion
ArraySpanFillFromScalarScratchSpace(*this->type),
```
##########
cpp/src/arrow/scalar.cc:
##########
@@ -716,18 +726,17 @@ RunEndEncodedScalar::RunEndEncodedScalar(const
std::shared_ptr<DataType>& type)
RunEndEncodedScalar::~RunEndEncodedScalar() = default;
-void RunEndEncodedScalar::FillScratchSpace() {
- auto run_end = run_end_type()->id();
+void RunEndEncodedScalar::FillScratchSpace(uint8_t* scratch_space, Type::type
run_end) {
switch (run_end) {
Review Comment:
```suggestion
void RunEndEncodedScalar::FillScratchSpace(uint8_t* scratch_space, const
DataType& type) {
switch (checked_cast<const RunEndEncodedType&>(type).run_end_type()->id())
{
```
--
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]
