Push filter down HashInnerJoin rule #4956

yixinglu · 2022-11-29T16:58:00Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

fix #4979

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

src/graph/optimizer/rule/PushFilterDownBiInnerJoinRule.cpp

tests/tck/features/optimizer/PushFilterDownBiInnerJoinRule.feature

tests/tck/features/match/AllShortestPaths.feature

jievince

Well done!

yixinglu · 2022-12-03T09:49:58Z

crashed LDBC query: use ldbc_v0_3_3

MATCH (country:Country) 
WHERE id(country) == "Spain" 
MATCH (a:Person)-[:IS_LOCATED_IN]->(:City)-[:IS_PART_OF]->(country) 
MATCH (b:Person)-[:IS_LOCATED_IN]->(:City)-[:IS_PART_OF]->(country) 
MATCH (c:Person)-[:IS_LOCATED_IN]->(:City)-[:IS_PART_OF]->(country) 
MATCH (a)-[:KNOWS]-(b), (b)-[:KNOWS]-(c), (c)-[:KNOWS]-(a) 
WHERE a.Person.id < b.Person.id AND b.Person.id < c.Person.id 
RETURN count(*) AS count

czpmango · 2022-12-06T06:19:35Z

src/graph/optimizer/OptGroup.cpp

+    if (!depGroup) {
+      return Status::Error("Could not find the dependent group in pattern leaves");
+    }
+    if (depGroup->groupNodes_.size() != 1U || depGroup->groupNodesReferenced_.size() != 1U) {
+      return Status::Error(
+          "Invalid sub-plan generated when applying the rule: %s, "
+          "planNode: %s, numGroupNodes: %lu, numGroupNodesRef: %lu",
+          rule->toString().c_str(),
+          PlanNode::toString(gn->node()->kind()),
+          depGroup->groupNodes_.size(),
+          depGroup->groupNodesReferenced_.size());
+    }


It is better not to pass optimizer errors to the user.

It might change this way:

DCHECK_NOTNULL(depGroup) << "Could not find the dependent group in pattern leaves";

or

// The `depGroup` should not be null bcz ... DCHECK_NOTNULL(depGroup);

I have converted the error message in QueryInstance. this return only for not crash in release version.

czpmango · 2022-12-06T06:21:44Z

src/graph/optimizer/OptGroup.cpp

@@ -41,9 +41,81 @@ OptGroup::OptGroup(OptContext *ctx) noexcept : ctx_(ctx) {
  DCHECK(ctx != nullptr);
 }

+Status OptGroup::validateSubPlan(const OptGroupNode *gn,


I can't find the call to this function. Did I miss something?

Good catch! Actually I don't use it in this PR except to debug some crash. I want to use it when refactoring go planner, now i need to consider loop/select plan nodes, it's a little tedious!

czpmango · 2022-12-06T06:35:52Z

src/graph/optimizer/rule/PushFilterDownHashInnerJoinRule.cpp

+  if (!filterPicked) return nullptr;
+
+  auto* newChildPlanNode = childPlanNode->clone();
+  DCHECK_NE(childPlanNode->outputVar(), newChildPlanNode->outputVar());


Just the behavior of PlanNode::PlanNode(), need DCHECK here?

Yeah, This line could be removed!

czpmango · 2022-12-06T06:42:16Z

src/graph/optimizer/rule/PushFilterDownHashInnerJoinRule.cpp

+  auto* newInnerJoinNode = static_cast<graph::HashInnerJoin*>(oldInnerJoinNode->clone());
+  auto newJoinGroup = rightFilterUnpicked ? OptGroup::create(octx) : filterGroupNode->group();
+  // TODO(yee): it's too tricky
+  auto newGroupNode = rightFilterUnpicked


Maybe renaming rightFilterUnpicked to filterRemained would be more readable.

czpmango · 2022-12-06T06:50:44Z

tests/tck/features/optimizer/PushFilterDownHashInnerJoinRule.feature

+# This source code is licensed under Apache 2.0 License.
+Feature: Push Filter down HashInnerJoin rule
+
+  Background:


Add test case:

match (a:player)-[:like]->(b) where b.player.age>30 match (b)-[:serve]->(c) where c.team.name>"A" and b.player.age+a.player.age>40 and a.player.age<45 return a,b,c

and

match (a:player)-[:like]->(b) where b.player.age>30 or b.player.age>45 match (b)-[:serve]->(c) where c.team.name>"A" or b.player.age+a.player.age>40 and a.player.age<45 return a,b,c

Test cases related to OR filter and non-query-part plan.

tests/tck/features/optimizer/PushFilterDownHashInnerJoinRule.feature

yixinglu · 2022-12-06T07:54:31Z

More cases will be added in NEXT PR.

yixinglu commented Nov 29, 2022

View reviewed changes

src/graph/optimizer/rule/PushFilterDownBiInnerJoinRule.cpp Outdated Show resolved Hide resolved

tests/tck/features/optimizer/PushFilterDownBiInnerJoinRule.feature Outdated Show resolved Hide resolved

yixinglu changed the title ~~Push filter down BiInnerJoin rule~~ Push filter down HashInnerJoin rule Dec 1, 2022

yixinglu force-pushed the push-filter-down-innerjoin branch 13 times, most recently from 724293e to 20a0593 Compare December 2, 2022 16:10

yixinglu commented Dec 3, 2022

View reviewed changes

tests/tck/features/match/AllShortestPaths.feature Outdated Show resolved Hide resolved

yixinglu force-pushed the push-filter-down-innerjoin branch 2 times, most recently from 332b6e0 to b829d79 Compare December 3, 2022 07:55

yixinglu added the ready-for-testing PR: ready for the CI test label Dec 3, 2022

yixinglu requested review from Shylock-Hg, nevermore3, Aiee, codesigner, czpmango, jievince and xtcyclist December 3, 2022 08:05

jievince previously approved these changes Dec 3, 2022

View reviewed changes

yixinglu dismissed jievince’s stale review via 03db518 December 3, 2022 09:48

yixinglu force-pushed the push-filter-down-innerjoin branch from 03db518 to 72218de Compare December 4, 2022 14:30

yixinglu added 9 commits December 6, 2022 11:42

Enhancement

3eccfc0

Disable the cleanup in optimizer

5fe1719

More DCHECK

160d917

More checks in optimizer

82e5a48

Use pattern leaves to validate the sub-plan

9c6bcf5

Rename BiInnerJoin to hash join

112e4bc

cleanup

9707c93

Add validator for OptGroup

e4ba34e

Fuck the bug

d7ff466

yixinglu force-pushed the push-filter-down-innerjoin branch from ae90172 to d7ff466 Compare December 6, 2022 03:42

cancel DCHECK

172509a

jievince previously approved these changes Dec 6, 2022

View reviewed changes

Fix tck tests

6aa04ab

yixinglu dismissed jievince’s stale review via 6aa04ab December 6, 2022 05:44

jievince approved these changes Dec 6, 2022

View reviewed changes

jievince mentioned this pull request Dec 6, 2022

push filter down traverse rule #4987

Merged

11 tasks

nevermore3 approved these changes Dec 6, 2022

View reviewed changes

czpmango reviewed Dec 6, 2022

View reviewed changes

Merge branch 'master' into push-filter-down-innerjoin

c918a8f

czpmango reviewed Dec 6, 2022

View reviewed changes

czpmango approved these changes Dec 6, 2022

View reviewed changes

yixinglu merged commit 7d27f32 into vesoft-inc:master Dec 6, 2022

yixinglu deleted the push-filter-down-innerjoin branch December 6, 2022 07:54

This was referenced Dec 6, 2022

Fix label tag expression toString function #5008

Merged

Throw the user-friendly message when error caused in optimizer of graphd #5033

Closed

wey-gu mentioned this pull request Dec 10, 2022

Weekly Report 2022-12-09 vesoft-inc/nebula-community#146

Closed

jievince mentioned this pull request Dec 23, 2022

Push Filter Down BiInnerJoin and Traverse PlanNode #4937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push filter down HashInnerJoin rule #4956

Push filter down HashInnerJoin rule #4956

yixinglu commented Nov 29, 2022 •

edited

Loading

jievince left a comment

yixinglu commented Dec 3, 2022 •

edited

Loading

czpmango Dec 6, 2022 •

edited

Loading

yixinglu Dec 6, 2022

czpmango Dec 6, 2022

czpmango Dec 6, 2022

yixinglu Dec 6, 2022

czpmango Dec 6, 2022

czpmango Dec 6, 2022

yixinglu Dec 6, 2022

czpmango Dec 6, 2022

czpmango Dec 6, 2022

czpmango Dec 6, 2022

yixinglu Dec 6, 2022

yixinglu commented Dec 6, 2022

Push filter down HashInnerJoin rule #4956

Push filter down HashInnerJoin rule #4956

Conversation

yixinglu commented Nov 29, 2022 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Release notes:

jievince left a comment

Choose a reason for hiding this comment

yixinglu commented Dec 3, 2022 • edited Loading

czpmango Dec 6, 2022 • edited Loading

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

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

yixinglu commented Dec 6, 2022

yixinglu commented Nov 29, 2022 •

edited

Loading

yixinglu commented Dec 3, 2022 •

edited

Loading

czpmango Dec 6, 2022 •

edited

Loading