Extend join fuzzer to cover group execution mode #9045
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
xiaoxmeng
force-pushed
the
fuzzer
branch
5 times, most recently
from
ef80610 to
417736c
Compare
xiaoxmeng
force-pushed
the
fuzzer
branch
3 times, most recently
from
df2bc59 to
70324ed
Compare
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
|
||
| private: | ||
| // Invoked to set up memory system with arbitration. | ||
| static void setupMemory() { |
There was a problem hiding this comment.
Could we use ArbitratorTestUtil::createMemoryManager here?
There was a problem hiding this comment.
@xiaoxmeng Would you answer this question?
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
velox/exec/tests/JoinFuzzer.cpp
Outdated
| size_t idx = randInt(0, kJoinTypes.size() - 1); | ||
| return kJoinTypes[idx]; | ||
| const size_t idx = randInt(0, kJoinTypes.size() - 1); | ||
| joinType_ = kJoinTypes[idx]; |
There was a problem hiding this comment.
Why is this change? It feels hard to maintain a large list of member variables with different methods changing some of these.
There was a problem hiding this comment.
@xiaoxmeng The approach looks good to me. There is a lot of code changes though. It is hard to follow these.
xiaoxmeng
force-pushed
the
fuzzer
branch
4 times, most recently
from
5327f41 to
cd12659
Compare
xiaoxmeng
force-pushed
the
fuzzer
branch
2 times, most recently
from
e7bee61 to
2352c41
Compare
velox/exec/tests/JoinFuzzer.cpp
Outdated
| builder.executionStrategy(core::ExecutionStrategy::kGrouped); | ||
| builder.groupedExecutionLeafNodeIds({plan.probeScanId, plan.buildScanId}); | ||
| builder.numSplitGroups(plan.numGroups); | ||
| builder.numConcurrentSplitGroups(1); |
There was a problem hiding this comment.
Would it makes sense to test with > 1 concurrent split group as well?
There was a problem hiding this comment.
@xiaoxmeng Looks good to me. Thank you for extending the Join Fuzzer to cover grouped execution. Please, run the fuzzer long enough to make sure there are no issues. Appreciate tidying up the code, but, please, next time avoid mixing cleanup / refactoring with material changes. It makes it difficult to review the changes.
xiaoxmeng
force-pushed
the
fuzzer
branch
3 times, most recently
from
3dcc9a8 to
8981e4d
Compare
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Extend join fuzzer to cover group execution mode. To do this we split the probe and build input by partitioning on the join keys with one partition per each group. Then we write the split the inputs into separate files and create table scan splits from the generated files. For each existing query plan with table scan input, we create a corresponding grouped execution plan. Extend AssertQueryBuilder to support group execution configuration.
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
2 similar comments
|
@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 merged this pull request in 3125512. |
Summary: Alternative join plans (including sort + merge join) were accidentaly disabled in a recent refactor (facebookincubator#9045). Re-enabling them in this PR. Differential Revision: D57696398
Summary: Alternative join plans (including sort + merge join) were accidentaly disabled in a recent refactor (facebookincubator#9045). Re-enabling them in this PR. Differential Revision: D57696398
) Summary: Extend join fuzzer to cover group execution mode. To do this we split the probe and build input by partitioning on the join keys with one partition per each group. Then we write the split the inputs into separate files and create table scan splits from the generated files. For each existing query plan with table scan input, we create a corresponding grouped execution plan. Extend AssertQueryBuilder to support group execution configuration. Pull Request resolved: facebookincubator#9045 Reviewed By: mbasmanova Differential Revision: D54792224 Pulled By: xiaoxmeng fbshipit-source-id: 79824df01a5468a4ebbd63316c1a046c3d4b92a1
Summary: Pull Request resolved: facebookincubator#9898 Alternative join plans (including sort + merge join) were accidentaly disabled in a recent refactor (facebookincubator#9045). Re-enabling them in this PR. Reviewed By: mbasmanova Differential Revision: D57696398 fbshipit-source-id: d0264a1fa214f0e1b46ab03b54fa76fd5fb59173
Summary: Pull Request resolved: facebookincubator#9898 Alternative join plans (including sort + merge join) were accidentaly disabled in a recent refactor (facebookincubator#9045). Re-enabling them in this PR. Reviewed By: mbasmanova Differential Revision: D57696398 fbshipit-source-id: d0264a1fa214f0e1b46ab03b54fa76fd5fb59173
Extend join fuzzer to cover group execution mode. To do this we split the probe and
build input by partitioning on the join keys with one partition per each group. Then we
write the split the inputs into separate files and create table scan splits from the generated
files. For each existing query plan with table scan input, we create a corresponding
grouped execution plan.
Extend AssertQueryBuilder to support group execution configuration.