fix

vesoft-inc · Dec 5, 2022 · 80a990c · 80a990c
1 parent b7c50de
commit 80a990c
Show file tree

Hide file tree

Showing 8 changed files with 140 additions and 52 deletions.
diff --git a/src/graph/optimizer/OptGroup.cpp b/src/graph/optimizer/OptGroup.cpp
@@ -47,7 +47,9 @@ void OptGroup::addGroupNode(OptGroupNode *groupNode) {
   if (outputVar_.empty()) {
     outputVar_ = groupNode->node()->outputVar();
   } else {
-    DCHECK_EQ(outputVar_, groupNode->node()->outputVar());
+    DCHECK_EQ(outputVar_, groupNode->node()->outputVar())
+        << "outputVar_: " << outputVar_ << "groupNode plan: " << groupNode->node()->toString()
+        << ", groupNode->node()->outputVar(): " << groupNode->node()->outputVar();
   }
   groupNodes_.emplace_back(groupNode);
   groupNode->node()->updateSymbols();

diff --git a/src/graph/optimizer/rule/PushFilterDownProjectRule.cpp b/src/graph/optimizer/rule/PushFilterDownProjectRule.cpp
@@ -24,6 +24,19 @@ PushFilterDownProjectRule::PushFilterDownProjectRule() {
   RuleSet::QueryRules().addRule(this);
 }
 
+bool PushFilterDownProjectRule::checkColumnExprKind(const Expression* expr) {
+  if (graph::ExpressionUtils::isPropertyExpr(expr)) {
+    return true;
+  }
+  if (expr->kind() == Expression::Kind::kSubscript) {
+    auto subscriptExpr = static_cast<const SubscriptExpression*>(expr);
+    if (graph::ExpressionUtils::isPropertyExpr(subscriptExpr->left())) {
+      return true;
+    }
+  }
+  return false;
+}
+
 const Pattern& PushFilterDownProjectRule::pattern() const {
   static Pattern pattern = Pattern::create(graph::PlanNode::Kind::kFilter,
                                            {Pattern::create(graph::PlanNode::Kind::kProject)});
@@ -48,28 +61,26 @@ StatusOr<OptRule::TransformResult> PushFilterDownProjectRule::transform(
   // Pick the passthrough expression items to avoid the expression overhead after filter pushdown
   auto picker = [&projColumns, &projColNames, &rewriteMap](const Expression* e) -> bool {
     auto varProps = graph::ExpressionUtils::collectAll(e,
-                                                       {Expression::Kind::kTagProperty,
-                                                        Expression::Kind::kEdgeProperty,
-                                                        Expression::Kind::kInputProperty,
-                                                        Expression::Kind::kVarProperty,
-                                                        Expression::Kind::kDstProperty,
-                                                        Expression::Kind::kSrcProperty});
+                                                       {
+                                                           Expression::Kind::kTagProperty,
+                                                           Expression::Kind::kEdgeProperty,
+                                                           Expression::Kind::kInputProperty,
+                                                           Expression::Kind::kVarProperty,
+                                                           Expression::Kind::kDstProperty,
+                                                           Expression::Kind::kSrcProperty,
+                                                       });
     if (varProps.empty()) {
       return false;
     }
-    std::vector<std::string> propNames;
-    for (auto* expr : varProps) {
-      DCHECK(graph::ExpressionUtils::isPropertyExpr(expr));
-      propNames.emplace_back(static_cast<const PropertyExpression*>(expr)->prop());
-    }
     for (size_t i = 0; i < projColNames.size(); ++i) {
       auto column = projColumns[i];
       auto colName = projColNames[i];
-      auto iter = std::find_if(propNames.begin(), propNames.end(), [&colName](const auto& name) {
-        return !colName.compare(name);
+      auto iter = std::find_if(varProps.begin(), varProps.end(), [&colName](const auto* expr) {
+        DCHECK(graph::ExpressionUtils::isPropertyExpr(expr));
+        return !colName.compare(static_cast<const PropertyExpression*>(expr)->prop());
       });
-      if (iter == propNames.end()) continue;
-      if (graph::ExpressionUtils::isPropertyExpr(column->expr())) {
+      if (iter == varProps.end()) continue;
+      if (checkColumnExprKind(column->expr())) {
         rewriteMap[colName] = column->expr();
         continue;
       } else {

diff --git a/src/graph/optimizer/rule/PushFilterDownProjectRule.h b/src/graph/optimizer/rule/PushFilterDownProjectRule.h
@@ -59,6 +59,8 @@ class PushFilterDownProjectRule final : public OptRule {
  private:
   PushFilterDownProjectRule();
 
+  static bool checkColumnExprKind(const Expression *expr);
+
   static std::unique_ptr<OptRule> kInstance;
 };
 

diff --git a/src/graph/optimizer/rule/PushFilterDownTraverseRule.cpp b/src/graph/optimizer/rule/PushFilterDownTraverseRule.cpp
@@ -37,6 +37,19 @@ const Pattern& PushFilterDownTraverseRule::pattern() const {
   return pattern;
 }
 
+bool PushFilterDownTraverseRule::match(OptContext* ctx, const MatchedResult& matched) const {
+  if (!OptRule::match(ctx, matched)) {
+    return false;
+  }
+  DCHECK_EQ(matched.dependencies[0].dependencies[0].node->node()->kind(),
+            PlanNode::Kind::kTraverse);
+  auto traverse =
+      static_cast<const Traverse*>(matched.dependencies[0].dependencies[0].node->node());
+  auto step = traverse->stepRange();
+  // step == nullptr means one step.
+  return step == nullptr;
+}
+
 StatusOr<OptRule::TransformResult> PushFilterDownTraverseRule::transform(
     OptContext* ctx, const MatchedResult& matched) const {
   auto* filterGroupNode = matched.node;
@@ -83,38 +96,42 @@ StatusOr<OptRule::TransformResult> PushFilterDownTraverseRule::transform(
   if (!filterPicked) {
     return TransformResult::noTransform();
   }
+  auto* newfilterPicked =
+      graph::ExpressionUtils::rewriteEdgePropertyFilter(pool, edgeAlias, filterPicked->clone());
 
   Filter* newFilter = nullptr;
   OptGroupNode* newFilterGroupNode = nullptr;
   if (filterUnpicked) {
     newFilter = Filter::make(qctx, nullptr, filterUnpicked);
-    // newFilter->setOutputVar(filter->outputVar());
-    // newFilter->setInputVar(filter->inputVar());
+    newFilter->setOutputVar(filter->outputVar());
+    newFilter->setColNames(filter->colNames());
     newFilterGroupNode = OptGroupNode::create(ctx, newFilter, filterGroup);
   }
 
   auto* newAv = static_cast<graph::AppendVertices*>(av->clone());
-  // newFilter->setInputVar(newAv->outputVar());
 
   OptGroupNode* newAvGroupNode = nullptr;
   if (newFilterGroupNode) {
     auto* newAvGroup = OptGroup::create(ctx);
     newAvGroupNode = newAvGroup->makeGroupNode(newAv);
     newFilterGroupNode->dependsOn(newAvGroup);
+    newFilter->setInputVar(newAv->outputVar());
   } else {
     newAvGroupNode = OptGroupNode::create(ctx, newAv, filterGroup);
+    newAv->setOutputVar(filter->outputVar());
   }
 
   auto* eFilter = tv->eFilter();
   Expression* newEFilter = nullptr;
   if (eFilter) {
-    auto logicExpr = LogicalExpression::makeAnd(pool, filterPicked, eFilter->clone());
+    auto logicExpr = LogicalExpression::makeAnd(pool, newfilterPicked, eFilter->clone());
     newEFilter = logicExpr;
   } else {
-    newEFilter = filterPicked;
+    newEFilter = newfilterPicked;
   }
 
   auto* newTv = static_cast<graph::Traverse*>(tv->clone());
+  newAv->setInputVar(newTv->outputVar());
   newTv->setEdgeFilter(newEFilter);
 
   auto* newTvGroup = OptGroup::create(ctx);

diff --git a/src/graph/optimizer/rule/PushFilterDownTraverseRule.h b/src/graph/optimizer/rule/PushFilterDownTraverseRule.h
@@ -14,6 +14,8 @@ class PushFilterDownTraverseRule final : public OptRule {
  public:
   const Pattern &pattern() const override;
 
+  bool match(OptContext *ctx, const MatchedResult &matched) const override;
+
   StatusOr<TransformResult> transform(OptContext *ctx, const MatchedResult &matched) const override;
 
   std::string toString() const override;

diff --git a/src/graph/util/ExpressionUtils.cpp b/src/graph/util/ExpressionUtils.cpp
@@ -1413,5 +1413,63 @@ bool ExpressionUtils::checkExprDepth(const Expression *expr) {
   return true;
 }
 
+// Transform expression `$-.e[0].likeness` to EdgePropertyExpression `like.likeness`
+// for more friendly to push down
+// \param pool object pool to hold ownership of objects alloacted
+// \param edgeAlias the name of edge. e.g. e in pattern -[e]->
+// \param expr the filter expression
+/*static*/ Expression *ExpressionUtils::rewriteEdgePropertyFilter(ObjectPool *pool,
+                                                                  const std::string &edgeAlias,
+                                                                  Expression *expr) {
+  graph::RewriteVisitor::Matcher matcher = [&edgeAlias](const Expression *e) -> bool {
+    if (e->kind() != Expression::Kind::kAttribute) {
+      return false;
+    }
+    auto attributeExpr = static_cast<const AttributeExpression *>(e);
+    auto *left = attributeExpr->left();
+    auto *right = attributeExpr->right();
+
+    if (left->kind() != Expression::Kind::kSubscript) return false;
+    if (right->kind() != Expression::Kind::kConstant ||
+        !static_cast<const ConstantExpression *>(right)->value().isStr())
+      return false;
+
+    auto subscriptExpr = static_cast<const SubscriptExpression *>(left);
+    auto *listExpr = subscriptExpr->left();
+    auto *idxExpr = subscriptExpr->right();
+    if (listExpr->kind() != Expression::Kind::kInputProperty &&
+        listExpr->kind() != Expression::Kind::kVarProperty) {
+      return false;
+    }
+    if (static_cast<const PropertyExpression *>(listExpr)->prop() != edgeAlias) {
+      return false;
+    }
+
+    // NOTE(jie): Just handled `$-.e[0].likeness` for now, whileas the traverse is single length
+    // expand.
+    // TODO(jie): Handle `ALL(i IN e WHERE i.likeness > 78)`, whileas the traverse is var len
+    // expand.
+    if (idxExpr->kind() != Expression::Kind::kConstant ||
+        static_cast<const ConstantExpression *>(idxExpr)->value() != 0) {
+      return false;
+    }
+
+    return true;
+  };
+  graph::RewriteVisitor::Rewriter rewriter = [pool](const Expression *e) -> Expression * {
+    DCHECK_EQ(e->kind(), Expression::Kind::kAttribute);
+    auto attributeExpr = static_cast<const AttributeExpression *>(e);
+    auto *right = attributeExpr->right();
+    DCHECK_EQ(right->kind(), Expression::Kind::kConstant);
+
+    auto &prop = static_cast<const ConstantExpression *>(right)->value().getStr();
+
+    auto *edgePropExpr = EdgePropertyExpression::make(pool, "*", prop);
+    return edgePropExpr;
+  };
+
+  return graph::RewriteVisitor::transform(expr, matcher, rewriter);
+}
+
 }  // namespace graph
 }  // namespace nebula
diff --git a/src/graph/util/ExpressionUtils.h b/src/graph/util/ExpressionUtils.h
@@ -225,6 +225,10 @@ class ExpressionUtils {
   // Whether the whole expression is vertex id predication
   // e.g. id(v) == 1, id(v) IN [...]
   static bool isVidPredication(const Expression* expr);
+
+  static Expression* rewriteEdgePropertyFilter(ObjectPool* pool,
+                                               const std::string& edgeAlias,
+                                               Expression* expr);
 };
 
 }  // namespace graph

diff --git a/tests/tck/features/optimizer/PushFilterDownTraverseRule.feature b/tests/tck/features/optimizer/PushFilterDownTraverseRule.feature
@@ -2,13 +2,9 @@
 #
 # This source code is licensed under Apache 2.0 License.
 Feature: Push Filter down Traverse rule
-  Examples:
-     | space_name  |
-     | nba         |
-     | nba_int_vid |
 
   Background:
-    Given a graph with space named "<space_name>"
+    Given a graph with space named "nba"
 
   Scenario: push filter down Traverse
     When profiling query:
@@ -18,43 +14,39 @@ Feature: Push Filter down Traverse rule
     Then the result should be, in any order:
       | e.likeness | v2.player.age |
       | 100        | 31            |
-      | 99         | 34            |
-      | 99         | 25            |
-      | 99         | 34            |
-      | 99         | 30            |
-      | 99         | 31            |
-      | 99         | 29            |
-      | 99         | 38            |
-      | 99         | 30            |
       | 100        | 43            |
-      | 99         | 88            |
-      | 99         | 32            |
-      | 99         | 42            |
-      | 99         | 33            |
-      | 99         | 41            |
+    # The filter `e.likeness>99` is first pushed down to the `eFilter_` of `Traverese` by rule `PushFilterDownTraverse`,
+    # and then pushed down to the `filter_` of `Traverse` by rule `PushEFilterDown`.
     And the execution plan should be:
-      | id | name           | dependencies | operator info                         |
-      | 7  | Project        | 6            |                                       |
-      | 6  | Project        | 5            |                                       |
-      | 5  | AppendVertices | 10           |                                       |
-      | 10 | Traverse       | 2            | {"edge filter": "(like.likeness>99)"} |
-      | 2  | Dedup          | 9            |                                       |
-      | 9  | IndexScan      | 3            |                                       |
-      | 3  | Start          |              |                                       |
+      | id | name           | dependencies | operator info                    |
+      | 6  | Project        | 5            |                                  |
+      | 5  | AppendVertices | 10           |                                  |
+      | 10 | Traverse       | 2            | {"filter": "(like.likeness>99)"} |
+      | 2  | Dedup          | 9            |                                  |
+      | 9  | IndexScan      | 3            |                                  |
+      | 3  | Start          |              |                                  |
 
   Scenario: push filter down Traverse with complex filter
     When profiling query:
       """
-      MATCH (v:player)-[e:like]->(v2) WHERE v.player.age > 35 and e.likeness > 99 RETURN e.likeness, v2.player.age
+      MATCH (v:player)-[e:like]->(v2) WHERE v.player.age != 35 and e.likeness != 99
+      RETURN e.likeness, v2.player.age as age
+      ORDER BY age
+      LIMIT 3
       """
     Then the result should be, in any order:
-      | e.likeness | v2.player.age |
-      | 100        | 31            |
+      | e.likeness | age |
+      | 90         | 20  |
+      | 80         | 22  |
+      | 90         | 23  |
+    # The filter `e.likeness!=99` is first pushed down to the `eFilter_` of `Traverese` by rule `PushFilterDownTraverse`,
+    # and then pushed down to the `filter_` of `Traverse` by rule `PushEFilterDown`.
     And the execution plan should be:
       | id | name           | dependencies | operator info                          |
+      | 11 | TopN           | 10           |                                        |
       | 10 | Project        | 9            |                                        |
-      | 9  | Filter         | 4            | {"condition": }                        |
+      | 9  | Filter         | 4            | {"condition": "(-.v.player.age!=35)" } |
       | 4  | AppendVertices | 12           |                                        |
-      | 12 | Traverse       | 8            | {"edge filter": "(like.likeness>99)" } |
+      | 12 | Traverse       | 8            | {"filter": "(like.likeness!=99)"}      |
       | 8  | IndexScan      | 2            |                                        |
       | 2  | Start          |              |                                        |