GH-45788: [C++][Acero] Fix data race in aggregate node #45789
Conversation
|
|
|
Hi @pitrou , would you like to take a look? Thanks. |
|
@github-actions crossbow submit emscripten |
|
Revision: 9d8ddbb Submitted crossbow builds: ursacomputing/crossbow @ actions-73916ce3ac
|
github-actions
bot
added
awaiting committer review
I guess not. Both arrow/cpp/src/arrow/compute/row/grouper.cc Line 163 in 7e18764 arrow/cpp/src/arrow/compute/row/grouper.cc Line 273 in 7e18764 |
| @@ -312,7 +312,7 @@ Result<ExecBatch> GroupByNode::Finalize() { | |||
| segment_key_field_ids_.size()); | |||
|
|
|||
| // Segment keys come first | |||
| PlaceFields(out_data, 0, segmenter_values_); | |||
| PlaceFields(out_data, 0, state->segmenter_values); | |||
There was a problem hiding this comment.
So, the Finalize step only considers the segmenter values for state[0]? I'm not sure I understand why.
| RETURN_NOT_OK( | ||
| ExtractSegmenterValues(&segmenter_values_, exec_batch, segment_field_ids_)); | ||
| RETURN_NOT_OK(ExtractSegmenterValues(&GetLocalState()->segmenter_values, exec_batch, | ||
| segment_field_ids_)); |
There was a problem hiding this comment.
OutputResult seems non-thread safe, how can InputReceived be called from several threads at once? Am I missing something?
Rationale for this change
Data race described in #45788 .
What changes are included in this PR?
Put the racing member
segmenter_valuesin thread local state.Are these changes tested?
Yes. UT added.
Are there any user-facing changes?
None.