Skip to content

Commit d702d0c

Browse files
authored
GH-45788: [C++][Acero] Fix data race in aggregate node (#45789)
### Rationale for this change Data race described in #45788 . ### What changes are included in this PR? Put the racing member `segmenter_values` in thread local state. ### Are these changes tested? Yes. UT added. ### Are there any user-facing changes? None. * GitHub Issue: #45788 Authored-by: Rossi Sun <zanmato1984@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent 5d487ca commit d702d0c

3 files changed

Lines changed: 42 additions & 2 deletions

File tree

cpp/src/arrow/acero/aggregate_internal.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,8 @@ Status ExtractSegmenterValues(std::vector<Datum>* values_ptr,
182182
const std::vector<int>& field_ids) {
183183
DCHECK_GT(input_batch.length, 0);
184184
std::vector<Datum>& values = *values_ptr;
185+
DCHECK_EQ(values.size(), field_ids.size());
185186
int64_t row = input_batch.length - 1;
186-
values.clear();
187-
values.resize(field_ids.size());
188187
for (size_t i = 0; i < field_ids.size(); i++) {
189188
const Datum& value = input_batch.values[field_ids[i]];
190189
if (value.is_scalar()) {

cpp/src/arrow/acero/aggregate_internal.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ class ScalarAggregateNode : public ExecNode, public TracedNode {
171171
TracedNode(this),
172172
segmenter_(std::move(segmenter)),
173173
segment_field_ids_(std::move(segment_field_ids)),
174+
segmenter_values_(segment_field_ids_.size()),
174175
target_fieldsets_(std::move(target_fieldsets)),
175176
aggs_(std::move(aggs)),
176177
kernels_(std::move(kernels)),
@@ -249,6 +250,7 @@ class GroupByNode : public ExecNode, public TracedNode {
249250
: ExecNode(input->plan(), {input}, {"groupby"}, std::move(output_schema)),
250251
TracedNode(this),
251252
segmenter_(std::move(segmenter)),
253+
segmenter_values_(segment_key_field_ids.size()),
252254
key_field_ids_(std::move(key_field_ids)),
253255
segment_key_field_ids_(std::move(segment_key_field_ids)),
254256
agg_src_types_(std::move(agg_src_types)),

cpp/src/arrow/acero/aggregate_node_test.cc

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,26 @@ TEST(GroupByNode, NoSkipNulls) {
213213
AssertExecBatchesEqualIgnoringOrder(out_schema, {expected_batch}, out_batches.batches);
214214
}
215215

216+
TEST(GroupByNode, BasicParallel) {
217+
const int64_t num_batches = 8;
218+
219+
std::vector<ExecBatch> batches(num_batches, ExecBatchFromJSON({int32()}, "[[42]]"));
220+
221+
Declaration plan = Declaration::Sequence(
222+
{{"exec_batch_source",
223+
ExecBatchSourceNodeOptions(schema({field("key", int32())}), batches)},
224+
{"aggregate", AggregateNodeOptions{/*aggregates=*/{{"hash_count_all", "count(*)"}},
225+
/*keys=*/{"key"}}}});
226+
227+
ASSERT_OK_AND_ASSIGN(BatchesWithCommonSchema out_batches,
228+
DeclarationToExecBatches(plan));
229+
230+
ExecBatch expected_batch = ExecBatchFromJSON(
231+
{int32(), int64()}, "[[42, " + std::to_string(num_batches) + "]]");
232+
AssertExecBatchesEqualIgnoringOrder(out_batches.schema, {expected_batch},
233+
out_batches.batches);
234+
}
235+
216236
TEST(ScalarAggregateNode, AnyAll) {
217237
// GH-43768: boolean_any and boolean_all with constant input should work well
218238
// when min_count != 0.
@@ -265,5 +285,24 @@ TEST(ScalarAggregateNode, AnyAll) {
265285
}
266286
}
267287

288+
TEST(ScalarAggregateNode, BasicParallel) {
289+
const int64_t num_batches = 8;
290+
291+
std::vector<ExecBatch> batches(num_batches, ExecBatchFromJSON({int32()}, "[[42]]"));
292+
293+
Declaration plan = Declaration::Sequence(
294+
{{"exec_batch_source",
295+
ExecBatchSourceNodeOptions(schema({field("", int32())}), batches)},
296+
{"aggregate", AggregateNodeOptions{/*aggregates=*/{{"count_all", "count(*)"}}}}});
297+
298+
ASSERT_OK_AND_ASSIGN(BatchesWithCommonSchema out_batches,
299+
DeclarationToExecBatches(plan));
300+
301+
ExecBatch expected_batch =
302+
ExecBatchFromJSON({int64()}, "[[" + std::to_string(num_batches) + "]]");
303+
AssertExecBatchesEqualIgnoringOrder(out_batches.schema, {expected_batch},
304+
out_batches.batches);
305+
}
306+
268307
} // namespace acero
269308
} // namespace arrow

0 commit comments

Comments
 (0)