Test NestedLoopJoin and MergeJoin in join fuzzer #9901

kagamiori · 2024-05-23T01:09:06Z

Summary:
A correctness bug was found in NestedLoopJoin recently (#9892),
so this diff adds NestedLoopJoin query plans with and without
TableScan to JoinFuzzer. It also adds MergeJoin with TableScan.

Differential Revision: D57703982

facebook-github-bot · 2024-05-23T01:09:17Z

This pull request was exported from Phabricator. Differential Revision: D57703982

netlify · 2024-05-23T01:09:24Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`9e22983`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/665612148d403700084842ab

) Summary: This diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Differential Revision: D57703982

facebook-github-bot · 2024-05-23T01:10:13Z

This pull request was exported from Phabricator. Differential Revision: D57703982

pedroerp · 2024-05-23T02:20:51Z

How does this relate to #9898 which also adds alternative plans (including merge join)?

kagamiori · 2024-05-23T03:43:56Z

How does this relate to #9898 which also adds alternative plans (including merge join)?

Hi @pedroerp, the merge join plan added in makeAlternativePlans() only reads data from Values node. This PR adds merge join plan that read from TableScan node.

) Summary: This diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Differential Revision: D57703982

facebook-github-bot · 2024-05-24T17:16:12Z

This pull request was exported from Phabricator. Differential Revision: D57703982

) Summary: A correctness bug was found in NestedLoopJoin recently (facebookincubator#9892), so this diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Differential Revision: D57703982

xiaoxmeng

@kagamiori thanks for the improvement % comments. Thanks!

xiaoxmeng · 2024-05-24T18:20:33Z

velox/exec/tests/NestedLoopJoinTest.cpp

@@ -131,6 +132,72 @@ class NestedLoopJoinTest : public HiveConnectorTestBase {
      buildKeyName_)};
 };

+TEST_F(NestedLoopJoinTest, aaa) {


velox/exec/NestedLoopJoinProbe.h

xiaoxmeng · 2024-05-24T18:46:52Z

velox/exec/fuzzer/JoinFuzzer.cpp

+std::string makeJoinFilter(
+    const std::vector<std::string>& probeKeys,
+    const std::vector<std::string>& buildKeys) {
+  std::string filter{};


const auto numKeys = probeKeys.size(); std::string filter;

xiaoxmeng · 2024-05-24T18:47:40Z

velox/exec/fuzzer/JoinFuzzer.cpp

@@ -510,6 +510,21 @@ core::PlanNodePtr tryFlipJoinSides(const core::HashJoinNode& joinNode) {
      joinNode.outputType());
 }

+core::PlanNodePtr tryFlipJoinSides(const core::NestedLoopJoinNode& joinNode) {


Can you name this tryFlipNestedJoinSides? And rename the existing one to tryFlipHashJoinSides? Thanks!

Hi @xiaoxmeng, I actually intentionally make the methods use the same name for both nested loop join and hash join. This is because I call tryFlipJoinSides() in addFlippedJoinPlan() that works for both join nodes.

xiaoxmeng · 2024-05-24T18:49:09Z

velox/exec/fuzzer/JoinFuzzer.cpp

@@ -813,6 +866,25 @@ void makeAlternativePlans(
                joinNode->joinType())
            .planNode()}});
  }
+
+  // Use NestedLoopJoin.
+  if (joinNode->isInnerJoin() || joinNode->isLeftJoin() ||


Can you put this into
addNestedJoinPlans? and the same for addMergeJoinPlans? thanks!

xiaoxmeng · 2024-05-24T18:49:27Z

velox/exec/fuzzer/JoinFuzzer.cpp

+  // Use NestedLoopJoin.
+  if (joinNode->isInnerJoin() || joinNode->isLeftJoin() ||
+      joinNode->isFullJoin()) {
+    auto filter = makeJoinFilter(probeKeys, buildKeys);


const auto filter

xiaoxmeng · 2024-05-24T18:53:15Z

velox/exec/fuzzer/JoinFuzzer.cpp

@@ -1114,6 +1186,67 @@ void JoinFuzzer::addPlansWithTableScan(
        groupedProbeScanSplits,
        groupedBuildScanSplits));
  }
+
+  // Add ungrouped MergeJoin with TableScan.
+  auto planNodeIdGenerator = std::make_shared<core::PlanNodeIdGenerator>();


Merge join and NestedLoopJoin doesn't work with grouped execution? Thanks!

No, grouped execution means the both input tables are already partitioned by the join key, so that one driver can join the partitions from both side on the same key value without knowing other partitions. Presto only supports hash join and hash aggregation according to the design doc (prestodb/presto#12124).

) Summary: A correctness bug was found in NestedLoopJoin recently (facebookincubator#9892), so this diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Differential Revision: D57703982

facebook-github-bot · 2024-05-25T00:03:00Z

This pull request was exported from Phabricator. Differential Revision: D57703982

xiaoxmeng

@kagamiori LGTM % comments. Thanks!

xiaoxmeng · 2024-05-25T02:26:06Z

velox/exec/fuzzer/JoinFuzzer.cpp

-                asRowType(joinNode->outputType())->names(),
-                joinNode->joinType())
-            .planNode()}});
+    const auto filter = makeJoinFilter(probeKeys, buildKeys);


This is not required?

Good catch. I'll remove it.

xiaoxmeng · 2024-05-25T02:28:45Z

velox/exec/fuzzer/JoinFuzzer.cpp

+        buildKeys,
+        probeScanSplits,
+        buildScanSplits,
+        outputColumns);


Do we want to flip the build/probe sides for merge join?

No, merge join only supports inner and left join. I've tried flipping but that's not allowed.
==>
Thinking twice, actually we can flip join sides for inner merge join. Let me add that.

) Summary: A correctness bug was found in NestedLoopJoin recently (facebookincubator#9892), so this diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Reviewed By: xiaoxmeng Differential Revision: D57703982

facebook-github-bot · 2024-05-25T20:55:24Z

This pull request was exported from Phabricator. Differential Revision: D57703982

) Summary: A correctness bug was found in NestedLoopJoin recently (facebookincubator#9892), so this diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Reviewed By: xiaoxmeng Differential Revision: D57703982

facebook-github-bot · 2024-05-25T20:55:47Z

This pull request was exported from Phabricator. Differential Revision: D57703982

…acebookincubator#9892) Summary: Join with no condition is a cross product. The existing code avoid adding mismatch to the result after cross product because cross product should have matched all input rows (facebookincubator#6010). But there is an exception. When the build or probe side is empty, this cross product is empty too. Hence for left, right, and full join, mismatch should still be produced. This diff fixes this bug by still adding the mismatch to the result if either build or probe side is empty. Reviewed By: Yuhta Differential Revision: D57681090

) Summary: A correctness bug was found in NestedLoopJoin recently (facebookincubator#9892), so this diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Reviewed By: xiaoxmeng Differential Revision: D57703982

facebook-github-bot · 2024-05-28T17:19:35Z

This pull request was exported from Phabricator. Differential Revision: D57703982

facebook-github-bot · 2024-05-28T20:53:44Z

This pull request has been merged in a06c53e.

conbench-facebook · 2024-05-28T21:17:51Z

Conbench analyzed the 1 benchmark run on commit a06c53e6.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

) Summary: Pull Request resolved: facebookincubator#9901 A correctness bug was found in NestedLoopJoin recently (facebookincubator#9892), so this diff adds NestedLoopJoin query plans with and without TableScan to JoinFuzzer. It also adds MergeJoin with TableScan. Reviewed By: xiaoxmeng Differential Revision: D57703982 fbshipit-source-id: 98d5d1cd6aa7a860e9b2bf1e1c10f9967f1c5dae

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 23, 2024

facebook-github-bot added the fb-exported label May 23, 2024

kagamiori force-pushed the export-D57703982 branch from 2ff937c to 0b7dd0d Compare May 23, 2024 01:10

mbasmanova requested review from xiaoxmeng and pedroerp May 23, 2024 11:37

kagamiori force-pushed the export-D57703982 branch from 0b7dd0d to b276cb0 Compare May 24, 2024 17:15

xiaoxmeng reviewed May 24, 2024

View reviewed changes

kagamiori force-pushed the export-D57703982 branch from b276cb0 to 465ae7d Compare May 25, 2024 00:02

kagamiori requested a review from xiaoxmeng May 25, 2024 00:41

xiaoxmeng approved these changes May 25, 2024

View reviewed changes

kagamiori force-pushed the export-D57703982 branch from 465ae7d to 6f8eb79 Compare May 25, 2024 20:55

kagamiori force-pushed the export-D57703982 branch from 6f8eb79 to 668f504 Compare May 25, 2024 20:55

kagamiori added 2 commits May 28, 2024 10:19

kagamiori force-pushed the export-D57703982 branch from 668f504 to 9e22983 Compare May 28, 2024 17:19

facebook-github-bot closed this in a06c53e May 28, 2024

facebook-github-bot added the Merged label May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test NestedLoopJoin and MergeJoin in join fuzzer #9901

Test NestedLoopJoin and MergeJoin in join fuzzer #9901

kagamiori commented May 23, 2024 •

edited

Loading

facebook-github-bot commented May 23, 2024

netlify bot commented May 23, 2024 •

edited

Loading

facebook-github-bot commented May 23, 2024

pedroerp commented May 23, 2024 •

edited

Loading

kagamiori commented May 23, 2024

facebook-github-bot commented May 24, 2024

xiaoxmeng left a comment

xiaoxmeng May 24, 2024

xiaoxmeng May 24, 2024

xiaoxmeng May 24, 2024

kagamiori May 24, 2024

xiaoxmeng May 24, 2024

xiaoxmeng May 24, 2024

xiaoxmeng May 24, 2024

kagamiori May 24, 2024 •

edited

Loading

facebook-github-bot commented May 25, 2024

xiaoxmeng left a comment

xiaoxmeng May 25, 2024

kagamiori May 25, 2024

xiaoxmeng May 25, 2024

kagamiori May 25, 2024 •

edited

Loading

facebook-github-bot commented May 25, 2024

facebook-github-bot commented May 25, 2024

facebook-github-bot commented May 28, 2024

facebook-github-bot commented May 28, 2024

conbench-facebook bot commented May 28, 2024

Test NestedLoopJoin and MergeJoin in join fuzzer #9901

Test NestedLoopJoin and MergeJoin in join fuzzer #9901

Conversation

kagamiori commented May 23, 2024 • edited Loading

facebook-github-bot commented May 23, 2024

netlify bot commented May 23, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented May 23, 2024

pedroerp commented May 23, 2024 • edited Loading

kagamiori commented May 23, 2024

facebook-github-bot commented May 24, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kagamiori May 24, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented May 25, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kagamiori May 25, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented May 25, 2024

facebook-github-bot commented May 25, 2024

facebook-github-bot commented May 28, 2024

facebook-github-bot commented May 28, 2024

conbench-facebook bot commented May 28, 2024

kagamiori commented May 23, 2024 •

edited

Loading

netlify bot commented May 23, 2024 •

edited

Loading

pedroerp commented May 23, 2024 •

edited

Loading

kagamiori May 24, 2024 •

edited

Loading

kagamiori May 25, 2024 •

edited

Loading