Fix infinite parallel join probe loop #7925
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
xiaoxmeng
force-pushed
the
parallel
branch
2 times, most recently
from
39c2aad
to
05479d7
Compare
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
xiaoxmeng
force-pushed
the
parallel
branch
4 times, most recently
from
ef95744
to
7c76b4f
Compare
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
velox/exec/HashTable.h
Outdated
| @@ -864,8 +896,8 @@ class HashTable : public BaseHashTable { | |||
| // many threads can set this. | |||
| std::atomic<bool> hasDuplicates_{false}; | |||
|
|
|||
| // Offset of next row link for join build side, 0 if none. Copied | |||
| // from 'rows_'. | |||
| // Offset of next row link for join build side, 0 if none. Copied from | |||
There was a problem hiding this comment.
What does "Copied from 'rows_'" means here? Does this need to be int64_t?
There was a problem hiding this comment.
I change to "set from 'rows_'". This is (relative) offset from a pointer so int32_t should be sufficient.
| std::vector<char*>* overflow) { | ||
| // The insertable rows are in the table, all get put in the hash | ||
| // table or array. | ||
| TableInsertPartitionInfo* partitionInfo) { |
There is infinite loop problem found in parallel table build in Meta internal test. If bucketOffset_ (int64_t) in ProbeState exceeds int32_t max limit, then HashTable::nextBucketOffset(int32_t) might cast it to a smaller value which cause fullProbe to restart the probe from a lower bucket offset and repeat this process forever. This PR fixes the problem by (1) change nextBucketOffset to take int64_t to int32_t to prevent offset integer overflow, and (2) fall back to the overflow insert path if the next bucket offset is below the starting bucket offset; (3) to prevent the similar bug to happen, we add to count the number of probed buckets and throws if the count exceeds the total number of buckets in the table. This fails a query but will not hang the driver thread. Also refactor the code by putting all the parallel join build related parameter into a TableInsertPartitionInfo data structure to simplify the implementation.
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@xiaoxmeng merged this pull request in bcd6652. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
There is infinite loop problem found in parallel table build in Meta internal test.
If bucketOffset_ (int64_t) in ProbeState exceeds int32_t max limit, then
HashTable::nextBucketOffset(int32_t) might cast it to a smaller value which cause
fullProbe to restart the probe from a lower bucket offset and repeat this process
forever.
This PR fixes the problem by (1) change nextBucketOffset to take int64_t to int32_t
to prevent offset integer overflow, and (2) fall back to the overflow insert path if the
next bucket offset is below the starting bucket offset; (3) to prevent the similar bug
to happen, we add to count the number of probed buckets and throws if the count
exceeds the total number of buckets in the table. This fails a query but will not hang
the driver thread.
Also refactor the code by putting all the parallel join build related parameter into a
TableInsertPartitionInfo data structure to simplify the implementation.