Fix double output for (Left|Right|Outer) without condition #6010

karteekmurthys · 2023-08-05T00:37:48Z

The NestedLoopJoin for Left/Right/Outer join without a condition is a cross join. Beyond that the code tries to find mismatched (build/probe) side rows to output will null values for outer join semantics.

If a filter is present, these mismatched rows SelectivityVector was set correctly depending on the filter results. However, if there is no filter present (the cross join case), the SelectivityVector was not updated in this code-path leading to all the rows being output a second time.

This PR fixes the problem by updating the SelectivityVector to indicate all rows are output already in the CrossJoin path.

Needed for prestodb/presto#20381 and fixes #5715

netlify · 2023-08-05T00:37:54Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`b70b7bc`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64d55f1bb191e8000860e4e0

velox/exec/NestedLoopJoinProbe.cpp

velox/exec/tests/NestedLoopJoinTest.cpp

aditi-pandit · 2023-08-05T02:18:21Z

velox/exec/tests/NestedLoopJoinTest.cpp

@@ -255,6 +255,64 @@ TEST_F(NestedLoopJoinTest, basicCrossJoin) {
      "SELECT * FROM t, (SELECT * FROM UNNEST (ARRAY[10, 17, 10, 17, 10, 17, 10, 17])) u");
 }

+TEST_F(NestedLoopJoinTest, crossJoinWithoutFilterAndCriteria) {


Nit : Rename test to outerJoinsWithoutCondition

velox/exec/tests/NestedLoopJoinTest.cpp

aditi-pandit · 2023-08-07T18:03:41Z

velox/exec/tests/NestedLoopJoinTest.cpp

+  createDuckDbTable("u", {buildVectors});
+
+  auto planNodeIdGenerator = std::make_shared<core::PlanNodeIdGenerator>();
+  auto executeOuterJoin = [&](core::JoinType joinType) {


Nit : rename testOuterJoin

mbasmanova · 2023-08-07T22:58:31Z

velox/exec/NestedLoopJoinProbe.cpp

@@ -401,6 +401,11 @@ RowVectorPtr NestedLoopJoinProbe::doMatch(vector_size_t probeCnt) {
  VELOX_CHECK(!hasProbedAllBuildData());

  if (joinCondition_ == nullptr) {
+    // All rows in SelectivityVector for probe and build must be marked valid
+    // for a CrossJoin.
+    probeMatched_.setAll();


It seems inefficient to reset unused SelectivityVectors. Should we look into modifying the needsProbeMismatch and needsBuildMismatch instead?

Agreed. An approach to support "left/right/full cross join" would be returning false from needsProbe/BuildMisatch when joinCondition_ == nullptr.

@mbasmanova, @usurai : A third option could be to change buildMismatchedOutput to return nullptr if joinCondition_ = nullptr
https://github.com/facebookincubator/velox/blob/main/velox/exec/NestedLoopJoinProbe.cpp#L225

It seemed needsProbe/BuildMismatch is well abstracted to just look at joinType to determine if mismatching is needed.

mbasmanova · 2023-08-08T13:22:25Z

CC: @usurai

usurai · 2023-08-08T15:06:24Z

I'd like to share my perspective regarding cross joins and their compatibility with different join types. While I understand your viewpoint that cross joins could potentially support left and right join types, I tend to lean towards the idea that cross joins should remain distinct and not be extended to encompass inner or outer join functionalities. This separation can help maintain the clarity and purpose of each type of join, ensuring that their distinctive roles are preserved within SQL queries.
With that in mind, my design around nested loop join was:

With join condition, the join is nested loop join that supports inner/outer join types.
Without join condition, the join is cross join (or cross product) and has not join type support. This is why the build function of cross join has no join type parameter:

velox/velox/exec/tests/utils/PlanBuilder.h

Lines 673 to 675 in 4b653ed

    
           PlanBuilder& nestedLoopJoin( 
        
               const core::PlanNodePtr& right, 
        
               const std::vector<std::string>& outputLayout);

Anyway, if cross join with outer join types is needed, update needsProbeMismatch and needsBuildMismatch to return false when joinCondition_ == nullptr would be enough and effcient.

aditi-pandit · 2023-08-08T17:43:31Z

I'd like to share my perspective regarding cross joins and their compatibility with different join types. While I understand your viewpoint that cross joins could potentially support left and right join types, I tend to lean towards the idea that cross joins should remain distinct and not be extended to encompass inner or outer join functionalities. This separation can help maintain the clarity and purpose of each type of join, ensuring that their distinctive roles are preserved within SQL queries. With that in mind, my design around nested loop join was:

With join condition, the join is nested loop join that supports inner/outer join types.

Without join condition, the join is cross join (or cross product) and has not join type support. This is why the build function of cross join has no join type parameter:

velox/velox/exec/tests/utils/PlanBuilder.h

Lines 673 to 675 in 4b653ed

PlanBuilder& nestedLoopJoin(

const core::PlanNodePtr& right,

const std::vector<std::string>& outputLayout);

Anyway, if cross join with outer join types is needed, update needsProbeMismatch and needsBuildMismatch to return false when joinCondition_ == nullptr would be enough and effcient.

Agree with @usurai that the current code is almost like 2 separate join cases put in the same operator. The bug was on account of the implementation of one side leaking to the other. It would be more clean to separate the CrossJoin from the outer joins with conditions.

aditi-pandit · 2023-08-08T23:51:56Z

velox/exec/NestedLoopJoinProbe.cpp

@@ -401,6 +403,11 @@ RowVectorPtr NestedLoopJoinProbe::doMatch(vector_size_t probeCnt) {
  VELOX_CHECK(!hasProbedAllBuildData());

  if (joinCondition_ == nullptr) {
+    // All rows in SelectivityVector for probe and build must be marked valid
+    // for a CrossJoin.
+    probeMatched_.setAll();


@karteekmurthys : These setAll(...) calls shouldn't be needed if we make the other change to needsProbe/BuildMismatch or add a check in buildMismatchedOutput.

velox/exec/NestedLoopJoinProbe.cpp

aditi-pandit · 2023-08-10T21:56:22Z

velox/exec/tests/NestedLoopJoinTest.cpp

@@ -255,6 +255,44 @@ TEST_F(NestedLoopJoinTest, basicCrossJoin) {
      "SELECT * FROM t, (SELECT * FROM UNNEST (ARRAY[10, 17, 10, 17, 10, 17, 10, 17])) u");
 }

+TEST_F(NestedLoopJoinTest, testOuterJoin) {


Maybe rename this testOuterJoinWithoutCondition so that the inner lambda can be renamed testOuterJoin ?

FYI: per naming convention, test methods should not have 'test' prefix

@mbasmanova : Yes, thanks my bad. I think this PR is done though. PTAL.

aditi-pandit · 2023-08-14T23:28:34Z

@usurai , @xiaoxmeng : Any chance you can take a look since Masha is on vacation ?

facebook-github-bot · 2023-08-16T06:19:11Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-08-16T17:52:57Z

@xiaoxmeng merged this pull request in 77e6d80.

conbench-facebook · 2023-08-16T18:18:09Z

Conbench analyzed the 1 benchmark run on commit 77e6d806.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…ncubator#6010) Summary: The NestedLoopJoin for Left/Right/Outer join without a condition is a cross join. Beyond that the code tries to find mismatched (build/probe) side rows to output will null values for outer join semantics. If a filter is present, these mismatched rows SelectivityVector was set correctly depending on the filter results. However, if there is no filter present (the cross join case), the SelectivityVector was not updated in this code-path leading to all the rows being output a second time. This PR fixes the problem by updating the SelectivityVector to indicate all rows are output already in the CrossJoin path. Needed for prestodb/presto#20381 and fixes facebookincubator#5715 Pull Request resolved: facebookincubator#6010 Reviewed By: Yuhta, amitkdutta Differential Revision: D48383119 Pulled By: xiaoxmeng fbshipit-source-id: 8a3725419d8fecd56827879ad6766cc479f468ac

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. Differential Revision: D57681090

…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. Differential Revision: D57681090

…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

…9892) Summary: Pull Request resolved: #9892 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 (#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 fbshipit-source-id: ac5960faf33166c4660bba25d516a2ffc1b6276c

…acebookincubator#9892) Summary: Pull Request resolved: facebookincubator#9892 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 fbshipit-source-id: ac5960faf33166c4660bba25d516a2ffc1b6276c

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 Aug 5, 2023

karteekmurthys requested a review from aditi-pandit August 5, 2023 00:37

karteekmurthys marked this pull request as draft August 5, 2023 00:38

karteekmurthys mentioned this pull request Aug 5, 2023

[native] Add support for LEFT, RIGHT and FULL joins with no equi clause prestodb/presto#20381

Merged

aditi-pandit changed the title ~~Fix matched rows for Cross Join without condition~~ Fix double output for (Left|Right|Outer) without condition Aug 5, 2023

aditi-pandit reviewed Aug 5, 2023

View reviewed changes

velox/exec/NestedLoopJoinProbe.cpp Outdated Show resolved Hide resolved

velox/exec/tests/NestedLoopJoinTest.cpp Show resolved Hide resolved

aditi-pandit reviewed Aug 5, 2023

View reviewed changes

karteekmurthys self-assigned this Aug 5, 2023

karteekmurthys force-pushed the nestedloop-join-fix branch from 7cdcb11 to 4111f25 Compare August 5, 2023 17:49

aditi-pandit reviewed Aug 7, 2023

View reviewed changes

karteekmurthys force-pushed the nestedloop-join-fix branch from 4111f25 to 7b3f478 Compare August 7, 2023 18:08

aditi-pandit marked this pull request as ready for review August 7, 2023 18:19

aditi-pandit requested a review from mbasmanova August 7, 2023 18:19

mbasmanova reviewed Aug 7, 2023

View reviewed changes

karteekmurthys force-pushed the nestedloop-join-fix branch from 7b3f478 to 1e926bd Compare August 8, 2023 17:19

karteekmurthys force-pushed the nestedloop-join-fix branch 2 times, most recently from 53ab6f4 to 6657052 Compare August 8, 2023 18:40

aditi-pandit reviewed Aug 8, 2023

View reviewed changes

karteekmurthys force-pushed the nestedloop-join-fix branch from 6657052 to 1034a63 Compare August 9, 2023 01:58

aditi-pandit reviewed Aug 9, 2023

View reviewed changes

velox/exec/NestedLoopJoinProbe.cpp Outdated Show resolved Hide resolved

karteekmurthys force-pushed the nestedloop-join-fix branch from 1034a63 to 7e06525 Compare August 10, 2023 21:40

aditi-pandit reviewed Aug 10, 2023

View reviewed changes

Fix matched rows for Cross Join without condition

b70b7bc

karteekmurthys force-pushed the nestedloop-join-fix branch from 7e06525 to b70b7bc Compare August 10, 2023 22:05

karteekmurthys requested a review from mbasmanova August 14, 2023 22:40

facebook-github-bot closed this in 77e6d80 Aug 16, 2023

facebook-github-bot added the Merged label Aug 16, 2023

karteekmurthys mentioned this pull request Aug 17, 2023

[native] Advance Velox version prestodb/presto#20583

Merged

6 tasks

kagamiori mentioned this pull request May 22, 2024

Fix NestedLoopJoin with empty build or probe with no join condition #9892

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix double output for (Left|Right|Outer) without condition #6010

Fix double output for (Left|Right|Outer) without condition #6010

karteekmurthys commented Aug 5, 2023 •

edited by aditi-pandit

Loading

netlify bot commented Aug 5, 2023 •

edited

Loading

aditi-pandit Aug 5, 2023

aditi-pandit Aug 7, 2023

mbasmanova Aug 7, 2023

usurai Aug 8, 2023 •

edited

Loading

aditi-pandit Aug 8, 2023 •

edited

Loading

mbasmanova commented Aug 8, 2023

usurai commented Aug 8, 2023

aditi-pandit commented Aug 8, 2023

aditi-pandit Aug 8, 2023

aditi-pandit Aug 10, 2023

mbasmanova Aug 10, 2023

aditi-pandit Aug 10, 2023 •

edited

Loading

aditi-pandit commented Aug 14, 2023

facebook-github-bot commented Aug 16, 2023

facebook-github-bot commented Aug 16, 2023

conbench-facebook bot commented Aug 16, 2023

Fix double output for (Left|Right|Outer) without condition #6010

Fix double output for (Left|Right|Outer) without condition #6010

Conversation

karteekmurthys commented Aug 5, 2023 • edited by aditi-pandit Loading

netlify bot commented Aug 5, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

aditi-pandit Aug 5, 2023

Choose a reason for hiding this comment

aditi-pandit Aug 7, 2023

Choose a reason for hiding this comment

mbasmanova Aug 7, 2023

Choose a reason for hiding this comment

usurai Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

aditi-pandit Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

mbasmanova commented Aug 8, 2023

usurai commented Aug 8, 2023

aditi-pandit commented Aug 8, 2023

aditi-pandit Aug 8, 2023

Choose a reason for hiding this comment

aditi-pandit Aug 10, 2023

Choose a reason for hiding this comment

mbasmanova Aug 10, 2023

Choose a reason for hiding this comment

aditi-pandit Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

aditi-pandit commented Aug 14, 2023

facebook-github-bot commented Aug 16, 2023

facebook-github-bot commented Aug 16, 2023

conbench-facebook bot commented Aug 16, 2023

karteekmurthys commented Aug 5, 2023 •

edited by aditi-pandit

Loading

netlify bot commented Aug 5, 2023 •

edited

Loading

usurai Aug 8, 2023 •

edited

Loading

aditi-pandit Aug 8, 2023 •

edited

Loading

aditi-pandit Aug 10, 2023 •

edited

Loading