Test NestedLoopJoin without filter in join fuzzer #9923
Conversation
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
✅ Deploy Preview for meta-velox canceled.
|
kagamiori
force-pushed
the
export-D57761514
branch
from
bf312cd
to
ef2732b
Compare
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. Differential Revision: D57761514
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
kagamiori
force-pushed
the
export-D57761514
branch
from
ef2732b
to
51ea92f
Compare
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Differential Revision: D57761514
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
51ea92f
to
e22fcb3
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
e22fcb3
to
3aab131
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
kagamiori
force-pushed
the
export-D57761514
branch
from
3aab131
to
cf1eba8
Compare
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Differential Revision: D57761514
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
cf1eba8
to
87e61eb
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
| for (auto i = 0; i < numRows; ++i) { | ||
| rowNumbers[i] = randInt(0, probe->size() - 1); | ||
| if (vectorFuzzer_.coinToss(0.8) && probe->size() > 0) { |
There was a problem hiding this comment.
Why we need the additional randomness processing here? Does it help catch any new bug? thanks!
There was a problem hiding this comment.
Hi @xiaoxmeng, yes, when generating a build input row, the original code randomly chooses an existing probe key value as the build key value. Then when the fuzzer tests right join and full join, there is always no unmatched rows from the build side. This PR makes the build input contain existing probe key values for 80% chance, and random build key values for the rest 20% so that there are some unmatched rows in the build input.
There was a problem hiding this comment.
Maybe comment this in the code. Thanks!
velox/exec/fuzzer/JoinFuzzer.cpp
Outdated
| const std::vector<std::string>& buildKeys, | ||
| const std::vector<RowVectorPtr>& probeInput, | ||
| const std::vector<RowVectorPtr>& buildInput) { | ||
| VELOX_CHECK(probeInput.size() > 0 && buildInput.size() > 0); |
There was a problem hiding this comment.
VELOX_CHECK_GE(probeInput.size(), 0);
VELOX_CHECK_GE(buildInput.size(), 0);
| VELOX_CHECK(probeInput.size() > 0 && buildInput.size() > 0); | ||
| const auto probeType = asRowType(probeInput[0]->type()); | ||
| const auto buildType = asRowType(buildInput[0]->type()); | ||
| auto outputColumns = |
There was a problem hiding this comment.
Can we generate the output columns as the other join type does?
There was a problem hiding this comment.
This is actually the same as how outputColumns is generated at line 1244 that is used for other join types. The code here is simpler because we already know the join type is not LeftSemiProjectJoin, LeftSemiFilterJoin, or AntiJoin. We only call testCrossProduct() when the join type is LeftJoin, FullJoin, or InnerJoin.
velox/exec/fuzzer/JoinFuzzer.cpp
Outdated
| buildKeys, | ||
| flatProbeInput, | ||
| flatBuildInput); | ||
| return; |
There was a problem hiding this comment.
Why we want to return here?
There was a problem hiding this comment.
testCrossProduct() executes a nested loop join with no join condition. Hash join and merge join cannot execute join with no join condition, so there is no need to continue executing alternative join plans for this iteration.
There was a problem hiding this comment.
Then shall we remove core::isInnerJoin(joinType) || core::isLeftJoin(joinType) in condition above? Thanks!
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
87e61eb
to
6f7ccbb
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
6f7ccbb
to
150bdea
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
2a21043
to
12f525d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
kagamiori
force-pushed
the
export-D57761514
branch
from
12f525d
to
833058f
Compare
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
833058f
to
0c18bd6
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
0c18bd6
to
3f5f1a2
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
3f5f1a2
to
54b954d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
54b954d
to
0b13cc8
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
…9923) Summary: A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514
kagamiori
force-pushed
the
export-D57761514
branch
from
0b13cc8
to
9a527c9
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57761514 |
|
This pull request has been merged in cda5d8a. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…9923) Summary: Pull Request resolved: facebookincubator#9923 A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514 fbshipit-source-id: 4a134ac315ea033ec1b66b874533b79c60437e07
…9923) Summary: Pull Request resolved: facebookincubator#9923 A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514 fbshipit-source-id: 4a134ac315ea033ec1b66b874533b79c60437e07
…9923) Summary: Pull Request resolved: facebookincubator#9923 A correctness bug was found recently in NestedLoopJoin without filter (facebookincubator#9892). So this diff extends join fuzzer to cover this case with 10% chance. This diff also makes the build side input to contain rows that do not match the probe side. Reviewed By: xiaoxmeng Differential Revision: D57761514 fbshipit-source-id: 4a134ac315ea033ec1b66b874533b79c60437e07
Summary:
A correctness bug was found recently in NestedLoopJoin without filter (#9892).
So this diff extends join fuzzer to cover this case with 10% chance.
Differential Revision: D57761514