From a7e6e026d3eaec6e4803d0c67d91912f532503b9 Mon Sep 17 00:00:00 2001 From: shylock <33566796+Shylock-Hg@users.noreply.github.com> Date: Fri, 11 Nov 2022 09:58:04 +0800 Subject: [PATCH] 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 | 11 +-- src/graph/util/ParserUtil.cpp | 67 ++++++++++++------- src/graph/validator/MatchValidator.cpp | 3 +- src/parser/MatchPath.h | 8 +++ tests/tck/features/bugfix/InnerVar.feature | 25 +++++++ .../PushFilterDownGetNbrsRule.feature | 8 +-- 6 files changed, 90 insertions(+), 32 deletions(-) create mode 100644 tests/tck/features/bugfix/InnerVar.feature diff --git a/src/graph/planner/match/MatchSolver.cpp b/src/graph/planner/match/MatchSolver.cpp index 1aa4e66ba6e..c888d910632 100644 --- a/src/graph/planner/match/MatchSolver.cpp +++ b/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); } }; diff --git a/src/graph/util/ParserUtil.cpp b/src/graph/util/ParserUtil.cpp index fe2727add76..ae8a8d30cca 100644 --- a/src/graph/util/ParserUtil.cpp +++ b/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(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(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(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(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(expr->clone()); + auto &matchPath = mpp->matchPath(); + for (auto &node : matchPath.nodes()) { + if (node->alias() == oldVarName) { + node->setAlias(newVarName); + } + } + return static_cast(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); diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index 0e36eff2854..c4d2dfb6cbd 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/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()); } } diff --git a/src/parser/MatchPath.h b/src/parser/MatchPath.h index f46970b2c12..c30a74df35c 100644 --- a/src/parser/MatchPath.h +++ b/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_; } diff --git a/tests/tck/features/bugfix/InnerVar.feature b/tests/tck/features/bugfix/InnerVar.feature new file mode 100644 index 00000000000..bff25d087d6 --- /dev/null +++ b/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" | diff --git a/tests/tck/features/optimizer/PushFilterDownGetNbrsRule.feature b/tests/tck/features/optimizer/PushFilterDownGetNbrsRule.feature index 358b2c23f21..5988b56d0dd 100644 --- a/tests/tck/features/optimizer/PushFilterDownGetNbrsRule.feature +++ b/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