Fix/inner variable race condition (#4850)

* Fix list comprehension.

* Fix hard code edge range inner variable.

* Run multiple times for tests.

src/graph/planner/match/MatchSolver.cpp

@@ -6,6 +6,7 @@
 #include "graph/planner/match/MatchSolver.h"
 
 #include "common/expression/UnaryExpression.h"
+#include "graph/context/QueryContext.h"
 #include "graph/context/ast/AstContext.h"
 #include "graph/context/ast/CypherAstContext.h"
 #include "graph/planner/Planner.h"
@@ -230,17 +231,19 @@ static YieldColumn* buildVertexColumn(ObjectPool* pool, const std::string& alias
   return new YieldColumn(InputPropertyExpression::make(pool, alias), alias);
 }
 
-static YieldColumn* buildEdgeColumn(ObjectPool* pool, const EdgeInfo& edge) {
+static YieldColumn* buildEdgeColumn(QueryContext* qctx, const EdgeInfo& edge) {
   Expression* expr = nullptr;
+  auto pool = qctx->objPool();
   if (edge.range == nullptr) {
     expr = SubscriptExpression::make(
         pool, InputPropertyExpression::make(pool, edge.alias), ConstantExpression::make(pool, 0));
   } else {
+    auto innerVar = qctx->vctx()->anonVarGen()->getVar();
     auto* args = ArgumentList::make(pool);
-    args->addArgument(VariableExpression::make(pool, "e"));
+    args->addArgument(VariableExpression::make(pool, innerVar));
     auto* filter = FunctionCallExpression::make(pool, "is_edge", args);
     expr = ListComprehensionExpression::make(
-        pool, "e", InputPropertyExpression::make(pool, edge.alias), filter);
+        pool, innerVar, InputPropertyExpression::make(pool, edge.alias), filter);
   }
   return new YieldColumn(expr, edge.alias);
 }
@@ -261,7 +264,7 @@ void MatchSolver::buildProjectColumns(QueryContext* qctx, Path& path, SubPlan& p
 
   auto addEdge = [columns, &colNames, qctx](auto& edgeInfo) {
     if (!edgeInfo.alias.empty() && !edgeInfo.anonymous) {
-      columns->addColumn(buildEdgeColumn(qctx->objPool(), edgeInfo));
+      columns->addColumn(buildEdgeColumn(qctx, edgeInfo));
       colNames.emplace_back(edgeInfo.alias);
     }
   };

src/graph/util/ParserUtil.cpp

@@ -4,6 +4,7 @@
 
 #include "graph/util/ParserUtil.h"
 
+#include "common/base/ObjectPool.h"
 #include "common/base/Status.h"
 #include "common/base/StatusOr.h"
 
@@ -14,41 +15,61 @@ namespace graph {
 void ParserUtil::rewriteLC(QueryContext *qctx,
                            ListComprehensionExpression *lc,
                            const std::string &oldVarName) {
-  qctx->vctx()->anonVarGen()->createVar(oldVarName);
-  qctx->ectx()->setValue(oldVarName, Value());
+  // The inner variable will be same with other inner variable in other expression,
+  // but the variable is stored in same variable map
+  // So to avoid conflict, we create a global unique anonymous variable name for it
+  // TODO store inner variable in inner
+  const auto &newVarName = qctx->vctx()->anonVarGen()->getVar();
+  qctx->ectx()->setValue(newVarName, Value());
   auto *pool = qctx->objPool();
 
   auto matcher = [](const Expression *expr) -> bool {
     return expr->kind() == Expression::Kind::kLabel ||
-           expr->kind() == Expression::Kind::kLabelAttribute;
+           expr->kind() == Expression::Kind::kLabelAttribute ||
+           expr->kind() == Expression::Kind::kMatchPathPattern;
   };
 
-  auto rewriter = [&, pool, oldVarName](const Expression *expr) {
+  auto rewriter = [&, pool, newVarName](const Expression *expr) {
     Expression *ret = nullptr;
-    if (expr->kind() == Expression::Kind::kLabel) {
-      auto *label = static_cast<const LabelExpression *>(expr);
-      if (label->name() == oldVarName) {
-        ret = VariableExpression::make(pool, oldVarName, true);
-      } else {
-        ret = label->clone();
-      }
-    } else {
-      DCHECK(expr->kind() == Expression::Kind::kLabelAttribute);
-      auto *la = static_cast<const LabelAttributeExpression *>(expr);
-      if (la->left()->name() == oldVarName) {
-        const auto &value = la->right()->value();
-        ret = AttributeExpression::make(pool,
-                                        VariableExpression::make(pool, oldVarName, true),
-                                        ConstantExpression::make(pool, value));
-      } else {
-        ret = la->clone();
-      }
+    switch (expr->kind()) {
+      case Expression::Kind::kLabel: {
+        auto *label = static_cast<const LabelExpression *>(expr);
+        if (label->name() == oldVarName) {
+          ret = VariableExpression::make(pool, newVarName, true);
+        } else {
+          ret = label->clone();
+        }
+      } break;
+      case Expression::Kind::kLabelAttribute: {
+        DCHECK(expr->kind() == Expression::Kind::kLabelAttribute);
+        auto *la = static_cast<const LabelAttributeExpression *>(expr);
+        if (la->left()->name() == oldVarName) {
+          const auto &value = la->right()->value();
+          ret = AttributeExpression::make(pool,
+                                          VariableExpression::make(pool, newVarName, true),
+                                          ConstantExpression::make(pool, value));
+        } else {
+          ret = la->clone();
+        }
+      } break;
+      case Expression::Kind::kMatchPathPattern: {
+        auto *mpp = static_cast<MatchPathPatternExpression *>(expr->clone());
+        auto &matchPath = mpp->matchPath();
+        for (auto &node : matchPath.nodes()) {
+          if (node->alias() == oldVarName) {
+            node->setAlias(newVarName);
+          }
+        }
+        return static_cast<Expression *>(mpp);
+      } break;
+      default:
+        LOG(FATAL) << "Unexpected expression kind: " << expr->kind();
     }
     return ret;
   };
 
   lc->setOriginString(lc->toString());
-  lc->setInnerVar(oldVarName);
+  lc->setInnerVar(newVarName);
   if (lc->hasFilter()) {
     Expression *filter = lc->filter();
     auto *newFilter = RewriteVisitor::transform(filter, matcher, rewriter);

src/graph/validator/MatchValidator.cpp

@@ -1103,7 +1103,8 @@ Status MatchValidator::validateMatchPathExpr(
 /*static*/ Status MatchValidator::buildRollUpPathInfo(const MatchPath *path, Path &pathInfo) {
   DCHECK(!DCHECK_NOTNULL(path->alias())->empty());
   for (const auto &node : path->nodes()) {
-    if (!node->alias().empty()) {
+    // The inner variable of expression will be replaced by anno variable
+    if (!node->alias().empty() && node->alias()[0] != '_') {
       pathInfo.compareVariables.emplace_back(node->alias());
     }
   }

src/parser/MatchPath.h

@@ -225,6 +225,10 @@ class MatchNode final {
   }
   MatchNode() = default;
 
+  void setAlias(const std::string& alias) {
+    alias_ = alias;
+  }
+
   const std::string& alias() const {
     return alias_;
   }
@@ -297,6 +301,10 @@ class MatchPath final {
     return alias_.get();
   }
 
+  auto& nodes() {
+    return nodes_;
+  }
+
   const auto& nodes() const {
     return nodes_;
   }

tests/tck/features/bugfix/InnerVar.feature

@@ -0,0 +1,25 @@
+# Copyright (c) 2022 vesoft inc. All rights reserved.
+#
+# This source code is licensed under Apache 2.0 License.
+Feature: Inner Var Conflict
+
+  Background:
+    Given a graph with space named "nba"
+
+  # The edge range will use same hard code innner variable name for list comprehension
+  # It's not stable to reproduce, so run multiple times
+  Scenario: Conflict hard code edge inner variable
+    When executing query 100 times:
+      """
+      MATCH (v)-[:like*1..2]->(v2) WHERE id(v) == 'Tim Duncan'
+      MATCH (v)-[:serve*1..2]->(t)
+      RETURN v.player.name, v2.player.name, t.team.name
+      """
+    Then the result should be, in any order:
+      | v.player.name | v2.player.name      | t.team.name |
+      | "Tim Duncan"  | "Tony Parker"       | "Spurs"     |
+      | "Tim Duncan"  | "Manu Ginobili"     | "Spurs"     |
+      | "Tim Duncan"  | "LaMarcus Aldridge" | "Spurs"     |
+      | "Tim Duncan"  | "Tim Duncan"        | "Spurs"     |
+      | "Tim Duncan"  | "Tim Duncan"        | "Spurs"     |
+      | "Tim Duncan"  | "Manu Ginobili"     | "Spurs"     |

tests/tck/features/optimizer/PushFilterDownGetNbrsRule.feature

@@ -72,10 +72,10 @@ Feature: Push Filter down GetNeighbors rule
       | "Manu Ginobili" | 95            |
       | "Tim Duncan"    | 95            |
     And the execution plan should be:
-      | id | name         | dependencies | operator info                                                |
-      | 0  | Project      | 1            |                                                              |
-      | 1  | GetNeighbors | 2            | {"filter": "(like.likeness IN [v IN [95,99] WHERE ($v>0)])"} |
-      | 2  | Start        |              |                                                              |
+      | id | name         | dependencies | operator info                                                            |
+      | 0  | Project      | 1            |                                                                          |
+      | 1  | GetNeighbors | 2            | {"filter": "(like.likeness IN [__VAR_0 IN [95,99] WHERE ($__VAR_0>0)])"} |
+      | 2  | Start        |              |                                                                          |
     When profiling query:
       """
       GO FROM "Tony Parker" OVER like