westonpace commented on a change in pull request #12437:
URL: https://github.com/apache/arrow/pull/12437#discussion_r809377866
##########
File path: cpp/src/arrow/compute/exec/hash_join.cc
##########
@@ -896,6 +896,17 @@ class HashJoinBasicImpl : public HashJoinImpl {
std::vector<uint8_t> has_match;
};
std::vector<ThreadLocalState> local_states_;
+ ThreadLocalState& GetLocalState(size_t thread_index) {
+ if (ARROW_PREDICT_FALSE(thread_index >= local_states_.size())) {
+ size_t old_size = local_states_.size();
+ local_states_.resize(thread_index + 1);
+ for (size_t i = old_size; i < local_states_.size(); ++i) {
+ local_states_[i].is_initialized = false;
+ local_states_[i].is_has_match_initialized = false;
+ }
+ }
+ return local_states_[thread_index];
+ }
Review comment:
The sizing is recomputed for each new exec plan so the failure would
only occur on plans that were running when the thread pool was resized. I am
planning on taking a look at a better implementation for use_threads=FALSE on
Friday using a serial executor which will ensure that an exec plan always has
an executor and exec plan steps are always run on a thread belonging to that
executor. This will solve all but the issue Antoine mentioned.
That being said, I think your solution is reasonable. I'll have to ping
@bkietz and @michalursa as they were the original proponents of the statically
sized thread states. I don't know if that was based on speculation, existing
literature, or actual benchmark measurements however.
--
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]
