[GitHub] [arrow] michalursa commented on pull request #13332: ARROW-16713: [C++] Pull join accumulation outside of HashJoinImpl

[GitBox <gi...@apache.org>]

michalursa commented on PR #13332:
URL: https://github.com/apache/arrow/pull/13332#issuecomment-1149110088

   Looks good to me.
   
   I have only a few comments:
   1. I would like ProbeBatches to be implemented outside of HashJoinImpl by calling ProbeSingleBatch method of HashJoinImpl. That way the implementation of ProbeBatches can be shared between both current HashJoinImpl and SwissJoin's implementation of it.
   2. finished_mutex_ in hash_join.cc is probably not used anymore and could be removed.
   3. I think I would prefer if AccumulationQueue was thread-safe than having HashJoinNode managing mutexes for accumulation queues. I see that in this implementation AccumulationQueue is sometimes used on a single-thread or in a read-only way, where the mutex is not necessary, but I would still prefer to extract a vector of exec batches in a thread-safe way from AccumulationQueue and use it instead in such situations. Or alternatively having a state flag in AccumulationQueue saying that it is read-only and doesn't require a mutex.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org