bkietz commented on code in PR #40237:
URL: https://github.com/apache/arrow/pull/40237#discussion_r1503000334
##########
cpp/src/arrow/array/data.cc:
##########
@@ -297,8 +299,10 @@ std::pair<BufferSpan, BufferSpan>
OffsetsAndSizesForScalar(uint8_t* scratch_spac
offset_type
value_size) {
auto* offsets = scratch_space;
auto* sizes = scratch_space + sizeof(offset_type);
- reinterpret_cast<offset_type*>(offsets)[0] = 0;
- reinterpret_cast<offset_type*>(sizes)[0] = value_size;
+ reinterpret_cast<std::atomic<offset_type>*>(offsets)->store(0,
+
std::memory_order_relaxed);
+ reinterpret_cast<std::atomic<offset_type>*>(sizes)->store(
+ static_cast<offset_type>(value_size), std::memory_order_relaxed);
Review Comment:
here too
```suggestion
auto* offsets = reinterpret_cast<std::atomic<offset_type>*>(scratch_space);
auto* sizes = offsets + 1;
offsets[0].store(0, std::memory_order_relaxed);
sizes[0].store(value_size, std::memory_order_relaxed);
```
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1984,4 +1985,41 @@ TEST_F(TestExtensionScalar, ValidateErrors) {
AssertValidationFails(scalar);
}
+template <typename T>
+class TestScalarScratchSpace : public ::testing::Test {
+ public:
+ TestScalarScratchSpace() = default;
+};
+
+TYPED_TEST_SUITE(TestScalarScratchSpace, BaseBinaryOrBinaryViewLikeArrowTypes);
+
+// GH-40069: race condition when filling the scratch space of a scalar in
parallel.
Review Comment:
Does this test reproduce the thread sanitizer failure on the thread
sanitizer CI job?
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1984,4 +1985,41 @@ TEST_F(TestExtensionScalar, ValidateErrors) {
AssertValidationFails(scalar);
}
+template <typename T>
+class TestScalarScratchSpace : public ::testing::Test {
+ public:
+ TestScalarScratchSpace() = default;
+};
+
+TYPED_TEST_SUITE(TestScalarScratchSpace, BaseBinaryOrBinaryViewLikeArrowTypes);
+
+// GH-40069: race condition when filling the scratch space of a scalar in
parallel.
+TYPED_TEST(TestScalarScratchSpace, ParallelFillScratchSpace) {
+ using ScalarType = typename TypeTraits<TypeParam>::ScalarType;
+
Review Comment:
Instead of using a test template, it should be simpler (especially for the
nested types) to give a list of scalars which should be tested:
```c++
for (auto scalar : {
ScalarFromJSON(utf8(), R"("test data")"),
ScalarFromJSON(utf8_view(), R"("test data")"),
ScalarFromJSON(list(int8()), "[1, 2, 3]"),
ScalarFromJSON(list_view(int8()), "[1, 2, 3]"),
}) {
...
}
```
##########
cpp/src/arrow/array/data.cc:
##########
@@ -286,8 +286,10 @@ namespace {
template <typename offset_type>
BufferSpan OffsetsForScalar(uint8_t* scratch_space, offset_type value_size) {
auto* offsets = reinterpret_cast<offset_type*>(scratch_space);
- offsets[0] = 0;
- offsets[1] = static_cast<offset_type>(value_size);
+ reinterpret_cast<std::atomic<offset_type>*>(&offsets[0])
+ ->store(0, std::memory_order_relaxed);
+ reinterpret_cast<std::atomic<offset_type>*>(&offsets[1])
+ ->store(static_cast<offset_type>(value_size), std::memory_order_relaxed);
Review Comment:
I think we can reduce the number of reinterpret casts here
```suggestion
auto* offsets = reinterpret_cast<std::atomic<offset_type>*>(scratch_space);
offsets[0].store(0, std::memory_order_relaxed);
offsets[1].store(value_size, std::memory_order_relaxed);
```
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1984,4 +1985,41 @@ TEST_F(TestExtensionScalar, ValidateErrors) {
AssertValidationFails(scalar);
}
+template <typename T>
+class TestScalarScratchSpace : public ::testing::Test {
+ public:
+ TestScalarScratchSpace() = default;
+};
+
+TYPED_TEST_SUITE(TestScalarScratchSpace, BaseBinaryOrBinaryViewLikeArrowTypes);
Review Comment:
This doesn't touch the List/ListView cases, please also test those
--
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]
