Fuck the bug

vesoft-inc · Dec 6, 2022 · d7ff466 · d7ff466
1 parent e4ba34e
commit d7ff466
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 29 deletions.
diff --git a/src/graph/optimizer/CMakeLists.txt b/src/graph/optimizer/CMakeLists.txt
@@ -27,7 +27,7 @@ nebula_add_library(
     rule/PushFilterDownAggregateRule.cpp
     rule/PushFilterDownProjectRule.cpp
     rule/PushFilterDownLeftJoinRule.cpp
-    # rule/PushFilterDownHashInnerJoinRule.cpp
+    rule/PushFilterDownHashInnerJoinRule.cpp
     rule/PushFilterDownInnerJoinRule.cpp
     rule/PushFilterDownNodeRule.cpp
     rule/PushFilterDownScanVerticesRule.cpp

diff --git a/src/graph/optimizer/OptGroup.cpp b/src/graph/optimizer/OptGroup.cpp
@@ -126,13 +126,9 @@ void OptGroup::addGroupNode(OptGroupNode *groupNode) {
 }
 
 OptGroupNode *OptGroup::makeGroupNode(PlanNode *node) {
-  if (outputVar_.empty()) {
-    outputVar_ = node->outputVar();
-  } else {
-    DCHECK_EQ(outputVar_, node->outputVar());
-  }
-  groupNodes_.emplace_back(OptGroupNode::create(ctx_, node, this));
-  return groupNodes_.back();
+  auto *gn = OptGroupNode::create(ctx_, node, this);
+  addGroupNode(gn);
+  return gn;
 }
 
 Status OptGroup::explore(const OptRule *rule) {
@@ -167,9 +163,15 @@ Status OptGroup::explore(const OptRule *rule) {
     NG_RETURN_IF_ERROR(resStatus);
     auto result = std::move(resStatus).value();
 
-    // for (auto *ngn : result.newGroupNodes) {
-    //   NG_RETURN_IF_ERROR(validateSubPlan(ngn, rule, leaves));
-    // }
+    for (auto *gn : result.newGroupNodes) {
+      auto it = std::find(groupNodes_.begin(), groupNodes_.end(), gn);
+      if (it != groupNodes_.end()) {
+        return Status::Error("Should not push the OptGroupNode %s into the group in the rule: %s",
+                             gn->node()->toString().c_str(),
+                             rule->toString().c_str());
+      }
+    }
+
     // In some cases, we can apply optimization rules even if the control flow and data flow are
     // inconsistent. For now, let the optimization rules themselves guarantee correctness.
     if (result.eraseAll) {
@@ -178,15 +180,13 @@ Status OptGroup::explore(const OptRule *rule) {
       }
       groupNodes_.clear();
       for (auto ngn : result.newGroupNodes) {
-        // NG_RETURN_IF_ERROR(validateSubPlan(ngn, rule, leaves));
         addGroupNode(ngn);
       }
       break;
     }
 
     if (!result.newGroupNodes.empty()) {
       for (auto ngn : result.newGroupNodes) {
-        // NG_RETURN_IF_ERROR(validateSubPlan(ngn, rule, leaves));
         addGroupNode(ngn);
       }
 

diff --git a/src/graph/optimizer/rule/PushFilterDownHashInnerJoinRule.cpp b/src/graph/optimizer/rule/PushFilterDownHashInnerJoinRule.cpp
@@ -64,7 +64,10 @@ StatusOr<OptRule::TransformResult> PushFilterDownHashInnerJoinRule::transform(
   // produce new InnerJoin node
   auto* newInnerJoinNode = static_cast<graph::HashInnerJoin*>(oldInnerJoinNode->clone());
   auto newJoinGroup = rightFilterUnpicked ? OptGroup::create(octx) : filterGroupNode->group();
-  auto newGroupNode = OptGroupNode::create(octx, newInnerJoinNode, newJoinGroup);
+  // TODO(yee): it's too tricky
+  auto newGroupNode = rightFilterUnpicked
+                          ? const_cast<OptGroup*>(newJoinGroup)->makeGroupNode(newInnerJoinNode)
+                          : OptGroupNode::create(octx, newInnerJoinNode, newJoinGroup);
   newGroupNode->dependsOn(leftGroup);
   newGroupNode->dependsOn(rightGroup);
   newInnerJoinNode->setLeftVar(leftGroup->outputVar());

diff --git a/src/graph/planner/plan/Query.cpp b/src/graph/planner/plan/Query.cpp
@@ -866,14 +866,15 @@ HashJoin::HashJoin(QueryContext* qctx,
     : BinaryInputNode(qctx, kind, left, right),
       hashKeys_(std::move(hashKeys)),
       probeKeys_(std::move(probeKeys)) {
-  auto lColNames = left->colNames();
-  auto& rColNames = right->colNames();
-  for (auto& rColName : rColNames) {
-    if (std::find(lColNames.begin(), lColNames.end(), rColName) == lColNames.end()) {
-      lColNames.emplace_back(rColName);
+  if (left && right) {
+    auto lColNames = left->colNames();
+    for (auto& rColName : right->colNames()) {
+      if (std::find(lColNames.begin(), lColNames.end(), rColName) == lColNames.end()) {
+        lColNames.emplace_back(rColName);
+      }
     }
+    setColNames(lColNames);
   }
-  setColNames(lColNames);
 }
 
 std::unique_ptr<PlanNodeDescription> HashLeftJoin::explain() const {
@@ -883,9 +884,7 @@ std::unique_ptr<PlanNodeDescription> HashLeftJoin::explain() const {
 }
 
 PlanNode* HashLeftJoin::clone() const {
-  auto* lnode = left() ? left()->clone() : nullptr;
-  auto* rnode = right() ? right()->clone() : nullptr;
-  auto* newLeftJoin = HashLeftJoin::make(qctx_, lnode, rnode);
+  auto* newLeftJoin = HashLeftJoin::make(qctx_, nullptr, nullptr);
   newLeftJoin->cloneMembers(*this);
   return newLeftJoin;
 }
@@ -901,9 +900,7 @@ std::unique_ptr<PlanNodeDescription> HashInnerJoin::explain() const {
 }
 
 PlanNode* HashInnerJoin::clone() const {
-  auto* lnode = left() ? left()->clone() : nullptr;
-  auto* rnode = right() ? right()->clone() : nullptr;
-  auto* newInnerJoin = HashInnerJoin::make(qctx_, lnode, rnode);
+  auto* newInnerJoin = HashInnerJoin::make(qctx_, nullptr, nullptr);
   newInnerJoin->cloneMembers(*this);
   return newInnerJoin;
 }
@@ -942,9 +939,7 @@ void RollUpApply::cloneMembers(const RollUpApply& r) {
 }
 
 PlanNode* RollUpApply::clone() const {
-  auto* lnode = left() ? left()->clone() : nullptr;
-  auto* rnode = right() ? right()->clone() : nullptr;
-  auto* newRollUpApply = RollUpApply::make(qctx_, lnode, rnode, {}, nullptr);
+  auto* newRollUpApply = RollUpApply::make(qctx_, nullptr, nullptr, {}, nullptr);
   newRollUpApply->cloneMembers(*this);
   return newRollUpApply;
 }