- Fix the setting of colNames and exchanging of PlanNodes in two filt…

…er push-down rules. (#4864)

- Added some tck tests.

src/graph/optimizer/rule/PushFilterDownAggregateRule.cpp

@@ -59,6 +59,7 @@ StatusOr<OptRule::TransformResult> PushFilterDownAggregateRule::transform(
   }
   std::unordered_map<std::string, Expression*> rewriteMap;
   auto colNames = newAggNode->colNames();
+  DCHECK_EQ(newAggNode->colNames().size(), newAggNode->groupItems().size());
   for (size_t i = 0; i < colNames.size(); ++i) {
     auto& colName = colNames[i];
     auto iter = std::find_if(propNames.begin(), propNames.end(), [&colName](const auto& name) {
@@ -89,11 +90,17 @@ StatusOr<OptRule::TransformResult> PushFilterDownAggregateRule::transform(
   newFilterNode->setCondition(newCondition);
 
   // Exchange planNode
+  // newAggNode shall inherit the output of the oldFilterNode
   newAggNode->setOutputVar(oldFilterNode->outputVar());
+  // as the new agg node now inherits the output var ptr from a filter node, the action of
+  // which alters its own colNames, its colNames need to be explicitly preserved.
+  newAggNode->setColNames(oldAggNode->colNames());
+  // newFilterNode shall inherit the input of the oldAggNode
   newFilterNode->setInputVar(oldAggNode->inputVar());
   DCHECK_EQ(oldAggNode->outputVar(), oldFilterNode->inputVar());
-  newAggNode->setInputVar(oldAggNode->outputVar());
-  newFilterNode->setOutputVar(oldAggNode->outputVar());
+  // newAggNode shall inherit oldFilterNode's inputs
+  newAggNode->setInputVar(oldFilterNode->inputVar());
+  newFilterNode->setOutputVar(newAggNode->inputVar());
 
   // Push down filter's optGroup and embed newAggGroupNode into old filter's
   // Group

src/graph/optimizer/rule/PushFilterDownProjectRule.cpp

@@ -136,13 +136,18 @@ StatusOr<OptRule::TransformResult> PushFilterDownProjectRule::transform(
     newAboveFilterNode->setInputVar(newProjNode->outputVar());
     result.newGroupNodes.emplace_back(newAboveFilterGroupNode);
   } else {
+    // newProjNode shall inherit the output from oldFilterNode
+    // which is the output of the opt group
     newProjNode->setOutputVar(oldFilterNode->outputVar());
+    // newProjNode's col names, on the hand, should inherit those of the oldProjNode
+    // since they are the same project.
+    newProjNode->setColNames(oldProjNode->outputVarPtr()->colNames);
     auto newProjGroupNode = OptGroupNode::create(octx, newProjNode, filterGroupNode->group());
     newProjGroupNode->setDeps({newBelowFilterGroup});
     newProjNode->setInputVar(newBelowFilterNode->outputVar());
     result.newGroupNodes.emplace_back(newProjGroupNode);
   }
-
+  DCHECK_EQ(newProjNode->colNames().size(), newProjNode->columns()->columns().size());
   return result;
 }
 

src/graph/planner/plan/PlanNode.cpp

@@ -396,6 +396,12 @@ SingleInputNode::SingleInputNode(QueryContext* qctx, Kind kind, const PlanNode*
   }
 }
 
+void SingleInputNode::copyInputColNames(const PlanNode* input) {
+  if (input != nullptr) {
+    setColNames(input->colNames());
+  }
+}
+
 std::unique_ptr<PlanNodeDescription> SingleDependencyNode::explain() const {
   auto desc = PlanNode::explain();
   DCHECK(desc->dependencies == nullptr);

src/graph/planner/plan/PlanNode.h

@@ -376,11 +376,7 @@ class SingleInputNode : public SingleDependencyNode {
     SingleDependencyNode::cloneMembers(node);
   }
 
-  void copyInputColNames(const PlanNode* input) {
-    if (input != nullptr) {
-      setColNames(input->colNames());
-    }
-  }
+  void copyInputColNames(const PlanNode* input);
 
   SingleInputNode(QueryContext* qctx, Kind kind, const PlanNode* dep);
 };

src/graph/planner/plan/Query.cpp

@@ -519,6 +519,15 @@ void Sample::cloneMembers(const Sample& l) {
   count_ = l.count_->clone();
 }
 
+Aggregate::Aggregate(QueryContext* qctx,
+                     PlanNode* input,
+                     std::vector<Expression*>&& groupKeys,
+                     std::vector<Expression*>&& groupItems)
+    : SingleInputNode(qctx, Kind::kAggregate, input) {
+  groupKeys_ = std::move(groupKeys);
+  groupItems_ = std::move(groupItems);
+}
+
 std::unique_ptr<PlanNodeDescription> Aggregate::explain() const {
   auto desc = SingleInputNode::explain();
   addDescription("groupKeys", folly::toJson(util::toJson(groupKeys_)), desc.get());

src/graph/planner/plan/Query.h

@@ -1169,11 +1169,7 @@ class Aggregate final : public SingleInputNode {
   Aggregate(QueryContext* qctx,
             PlanNode* input,
             std::vector<Expression*>&& groupKeys,
-            std::vector<Expression*>&& groupItems)
-      : SingleInputNode(qctx, Kind::kAggregate, input) {
-    groupKeys_ = std::move(groupKeys);
-    groupItems_ = std::move(groupItems);
-  }
+            std::vector<Expression*>&& groupItems);
 
   void cloneMembers(const Aggregate&);
 

src/graph/visitor/PrunePropertiesVisitor.cpp

@@ -57,7 +57,8 @@ void PrunePropertiesVisitor::visitCurrent(Project *node) {
     rootNode_ = false;
     return;
   }
-  for (auto i = 0u; i < columns.size(); ++i) {
+  DCHECK_EQ(columns.size(), colNames.size());
+  for (auto i = 0u; i < columns.size() && i < colNames.size(); ++i) {
     auto *col = DCHECK_NOTNULL(columns[i]);
     auto *expr = col->expr();
     auto &alias = colNames[i];

tests/tck/features/bugfix/MatchCrash.feature

@@ -0,0 +1,18 @@
+# Copyright (c) 2022 vesoft inc. All rights reserved.
+#
+# This source code is licensed under Apache 2.0 License.
+Feature: match bug fix
+
+  Background:
+    Given a graph with space named "nba"
+
+  Scenario: match crash due to where in with
+    When try to execute query:
+      """
+      MATCH (n0)-[e0]->(n1:player{age: 102, in_service: false})
+      WHERE (id(n0) IN ["Tim Duncan"])
+      WITH MIN(87) AS a0, n1.player.served_years AS a1
+      WHERE a1 == 100
+      RETURN *
+      """
+    Then the execution should be successful

tests/tck/features/optimizer/PushFilterDownAggregateRule.feature

@@ -6,8 +6,6 @@ Feature: Push Filter down Aggregate rule
   Background:
     Given a graph with space named "nba"
 
-  # TODO(yee):
-  @skip
   Scenario: push filter down Aggregate
     When profiling query:
       """
@@ -35,14 +33,51 @@ Feature: Push Filter down Aggregate rule
       | 28  | 1     |
       | 29  | 3     |
     And the execution plan should be:
-      | id | name        | dependencies | operator info |
-      | 12 | Sort        | 19           |               |
-      | 19 | Aggregate   | 18           |               |
-      | 18 | Filter      | 8            |               |
-      | 8  | Filter      | 7            |               |
-      | 7  | Project     | 6            |               |
-      | 6  | Project     | 5            |               |
-      | 5  | Filter      | 17           |               |
-      | 17 | GetVertices | 14           |               |
-      | 14 | IndexScan   | 0            |               |
-      | 0  | Start       |              |               |
+      | id | name           | dependencies | operator info |
+      | 8  | Sort           | 13           |               |
+      | 13 | Aggregate      | 10           |               |
+      | 10 | Filter         | 12           |               |
+      | 12 | AppendVertices | 1            |               |
+      | 1  | IndexScan      | 2            |               |
+      | 2  | Start          |              |               |
+
+  Scenario: push filter down aggregate and project
+    When profiling query:
+      """
+      match (n0)-[e:like]->(n1:player{name: "Tim Duncan"})
+      where id(n1) == "Tim Duncan"
+      with min(87) as n0, n1.player.age as age
+      where age > 10
+      return *
+      """
+    Then the result should be, in any order:
+      | n0 | age |
+      | 87 | 42  |
+    And the execution plan should be:
+      | id | name           | dependencies | operator info |
+      | 15 | Aggregate      | 13           |               |
+      | 13 | Project        | 12           |               |
+      | 12 | Filter         | 5            |               |
+      | 5  | AppendVertices | 14           |               |
+      | 14 | Traverse       | 2            |               |
+      | 2  | Dedup          | 1            |               |
+      | 1  | PassThrough    | 3            |               |
+      | 3  | Start          |              |               |
+
+  Scenario: push multiple filters down aggregate
+    When executing query:
+      """
+      match (v0:player)-[e0:serve]->(v1)<-[e1:serve]-(v2:player)
+      where id(v0) in ["Tim Duncan", "Aron Baynes", "Ben Simmons"]
+      with v0, avg(v0.player.age + e0.start_year + v2.player.age + 1000) as pi0,
+      endNode(e0) as pi1
+      with (v0.player.age + 1000 ) / pi1.player.age as pi0, pi1, pi1.player.age + 50 as pi2,
+      v0, pi1.player.name as pi3, avg(v0.player.age * 100 + pi0) as pi4
+      where v0.player.name == "Ben Simmons"
+      with (v0.player.age + 1000 ) / pi1.player.age as pi0, pi1, pi1.player.age + 50 as pi2,
+      v0, pi1.player.name as pi3, avg(v0.player.age * 100 + pi0) as pi4
+      where v0.player.name == "Ben Simmons"
+      with v0.player.name as pi0 where pi0 == pi0
+      return *
+      """
+    Then the execution should be successful

tests/tck/features/optimizer/PushFilterDownProjectRule.feature

@@ -7,7 +7,6 @@ Feature: Push Filter down Project rule
     Given a graph with space named "nba"
 
   # TODO(yee):
-  @skip
   Scenario: push filter down Project
     When profiling query:
       """
@@ -23,25 +22,14 @@ Feature: Push Filter down Project rule
       | ("Luka Doncic" :player{age: 20, name: "Luka Doncic"})               |
       | ("Luka Doncic" :player{age: 20, name: "Luka Doncic"})               |
     And the execution plan should be:
-      | id | name         | dependencies | operator info |
-      | 23 | Project      | 40           |               |
-      | 40 | Project      | 39           |               |
-      | 39 | Filter       | 20           |               |
-      | 20 | Filter       | 19           |               |
-      | 19 | Project      | 18           |               |
-      | 18 | InnerJoin    | 17           |               |
-      | 17 | Project      | 28           |               |
-      | 28 | GetVertices  | 13           |               |
-      | 13 | InnerJoin    | 12           |               |
-      | 12 | Filter       | 11           |               |
-      | 11 | Project      | 32           |               |
-      | 32 | GetNeighbors | 7            |               |
-      | 7  | Filter       | 6            |               |
-      | 6  | Project      | 5            |               |
-      | 5  | Filter       | 31           |               |
-      | 31 | GetNeighbors | 24           |               |
-      | 24 | IndexScan    | 0            |               |
-      | 0  | Start        |              |               |
+      | id | name           | dependencies | operator info |
+      | 14 | Project        | 11           |               |
+      | 11 | Filter         | 5            |               |
+      | 5  | AppendVertices | 4            |               |
+      | 4  | Traverse       | 13           |               |
+      | 13 | Traverse       | 1            |               |
+      | 1  | IndexScan      | 2            |               |
+      | 2  | Start          |              |               |
     When profiling query:
       """
       MATCH (a:player)--(b)--(c)
@@ -60,22 +48,12 @@ Feature: Push Filter down Project rule
       | ("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"}) | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"})       | 35   |
       | ("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"}) | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"})       | 30   |
     And the execution plan should be:
-      | id | name         | dependencies | operator info |
-      | 24 | Dedup        | 41           |               |
-      | 41 | Project      | 40           |               |
-      | 40 | Filter       | 20           |               |
-      | 20 | Filter       | 19           |               |
-      | 19 | Project      | 18           |               |
-      | 18 | InnerJoin    | 17           |               |
-      | 17 | Project      | 29           |               |
-      | 29 | GetVertices  | 13           |               |
-      | 13 | InnerJoin    | 12           |               |
-      | 12 | Filter       | 11           |               |
-      | 11 | Project      | 33           |               |
-      | 33 | GetNeighbors | 7            |               |
-      | 7  | Filter       | 6            |               |
-      | 6  | Project      | 5            |               |
-      | 5  | Filter       | 32           |               |
-      | 32 | GetNeighbors | 26           |               |
-      | 26 | IndexScan    | 0            |               |
-      | 0  | Start        |              |               |
+      | id | name           | dependencies | operator info |
+      | 10 | Dedup          | 15           |               |
+      | 15 | Project        | 12           |               |
+      | 12 | Filter         | 5            |               |
+      | 5  | AppendVertices | 4            |               |
+      | 4  | Traverse       | 14           |               |
+      | 14 | Traverse       | 1            |               |
+      | 1  | IndexScan      | 2            |               |
+      | 2  | Start          |              |               |