Skip to content

Commit

Permalink
Fix expressions that shall be rewritten to LabelTagProperty (#5054)
Browse files Browse the repository at this point in the history 
Rewrite attribute expresion to labeltagprop for unwind. Fix other related cases such as ListComprehensionExpression, reduce, etc.
  • Loading branch information
@xtcyclist
xtcyclist authored Dec 21, 2022
1 parent fd46ba9 commit 8cfc0bc
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 52 deletions.
42 changes: 38 additions & 4 deletions src/common/expression/AttributeExpression.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,48 @@ const Value &AttributeExpression::eval(ExpressionContext &ctx) {
case Value::Type::MAP:
return lvalue.getMap().at(rvalue.getStr());
case Value::Type::VERTEX: {
/*
* WARNING(Xuntao): Vertices shall not be evaluated as AttributeExpressions,
* since there shall always be a tag specified in the format of:
* var.tag.property. Due to design flaws, however, we have to keep
* this case segment.
*/
if (rvalue.getStr() == kVid) {
result_ = lvalue.getVertex().vid;
return result_;
}
for (auto &tag : lvalue.getVertex().tags) {
auto iter = tag.props.find(rvalue.getStr());
if (iter != tag.props.end()) {
return iter->second;
/*
* WARNING(Xuntao): the following code snippet is dedicated to address the legacy
* problem of treating LabelTagProperty expressions as Attribute expressions.
* This snippet is necessary to allow users to write var.tag.prop in
* ListComprehensionExpression without making structural changes to the implementation.
*/
if (left()->kind() != Expression::Kind::kAttribute) {
if (right()->kind() == Expression::Kind::kConstant &&
rvalue.type() == Value::Type::STRING) {
auto rStr = rvalue.getStr();
for (auto &tag : lvalue.getVertex().tags) {
if (rStr.compare(tag.name) == 0) {
return lvalue;
}
}
LOG(ERROR) << "Tag not found for: " << rStr
<< "Please check whether the related expression "
<< "follows the format of vertex.tag.property.";
}
} else if (left()->kind() == Expression::Kind::kAttribute &&
dynamic_cast<AttributeExpression *>(left())->right()->kind() ==
Expression::Kind::kConstant) {
auto &tagName = dynamic_cast<AttributeExpression *>(left())->right()->eval(ctx).getStr();
for (auto &tag : lvalue.getVertex().tags) {
if (tagName.compare(tag.name) != 0) {
continue;
} else {
auto iter = tag.props.find(rvalue.getStr());
if (iter != tag.props.end()) {
return iter->second;
}
}
}
}
return Value::kNullValue;
Expand Down
48 changes: 34 additions & 14 deletions src/common/expression/test/AttributeExpressionTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
*
* This source code is licensed under Apache 2.0 License.
*/
#include <unordered_map>

#include "common/expression/test/TestBase.h"
#include "graph/util/ExpressionUtils.h"

namespace nebula {

Expand Down Expand Up @@ -82,6 +85,8 @@ TEST_F(AttributeExpressionTest, VertexAttribute) {
Vertex vertex;
vertex.vid = "vid";
vertex.tags.resize(2);
vertex.tags[0].name = "tag0";
vertex.tags[1].name = "tag1";
vertex.tags[0].props = {
{"Venus", "Mars"},
{"Mull", "Kintyre"},
Expand All @@ -91,35 +96,50 @@ TEST_F(AttributeExpressionTest, VertexAttribute) {
{"Tug", "War"},
{"Venus", "RocksShow"},
};
{
auto *left = ConstantExpression::make(&pool, Value(vertex));
auto *right = LabelExpression::make(&pool, "Mull");
auto expr = AttributeExpression::make(&pool, left, right);
auto value = Expression::eval(expr, gExpCtxt);
std::unordered_map<std::string, graph::AliasType> aliasTypeMap = {{"v", graph::AliasType::kNode}};
ExpressionContextMock expContext;
expContext.setVar("v", Value(vertex));
{
auto *left = LabelExpression::make(&pool, "v");
auto *right = ConstantExpression::make(&pool, "tag0");
auto *labelAttribute = LabelAttributeExpression::make(&pool, left, right);
auto expr =
AttributeExpression::make(&pool, labelAttribute, ConstantExpression::make(&pool, "Mull"));
auto rewriteExpr =
graph::ExpressionUtils::rewriteAttr2LabelTagProp(expr->clone(), aliasTypeMap);
auto value = Expression::eval(rewriteExpr, expContext);
ASSERT_TRUE(value.isStr());
ASSERT_EQ("Kintyre", value.getStr());
}
{
auto *left = ConstantExpression::make(&pool, Value(vertex));
auto *right = LabelExpression::make(&pool, "Bip");
auto expr = AttributeExpression::make(&pool, left, right);
auto value = Expression::eval(expr, gExpCtxt);
auto *left = LabelExpression::make(&pool, "v");
auto *right = ConstantExpression::make(&pool, "tag1");
auto *labelAttribute = LabelAttributeExpression::make(&pool, left, right);
auto expr =
AttributeExpression::make(&pool, labelAttribute, ConstantExpression::make(&pool, "Bip"));
auto rewriteExpr =
graph::ExpressionUtils::rewriteAttr2LabelTagProp(expr->clone(), aliasTypeMap);
auto value = Expression::eval(rewriteExpr, expContext);
ASSERT_TRUE(value.isStr());
ASSERT_EQ("Bop", value.getStr());
}
{
auto *left = ConstantExpression::make(&pool, Value(vertex));
auto *right = LabelExpression::make(&pool, "Venus");
auto expr = AttributeExpression::make(&pool, left, right);
auto value = Expression::eval(expr, gExpCtxt);
auto *left = LabelExpression::make(&pool, "v");
auto *right = ConstantExpression::make(&pool, "tag0");
auto *labelAttribute = LabelAttributeExpression::make(&pool, left, right);
auto expr =
AttributeExpression::make(&pool, labelAttribute, ConstantExpression::make(&pool, "Venus"));
auto rewriteExpr =
graph::ExpressionUtils::rewriteAttr2LabelTagProp(expr->clone(), aliasTypeMap);
auto value = Expression::eval(rewriteExpr, expContext);
ASSERT_TRUE(value.isStr());
ASSERT_EQ("Mars", value.getStr());
}
{
auto *left = ConstantExpression::make(&pool, Value(vertex));
auto *right = LabelExpression::make(&pool, "_vid");
auto expr = AttributeExpression::make(&pool, left, right);
auto value = Expression::eval(expr, gExpCtxt);
auto value = Expression::eval(expr, expContext);
ASSERT_TRUE(value.isStr());
ASSERT_EQ("vid", value.getStr());
}
Expand Down
13 changes: 11 additions & 2 deletions src/common/expression/test/ExpressionContextMock.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,16 @@ class ExpressionContextMock final : public ExpressionContext {
}

void setInnerVar(const std::string& var, Value val) override {
exprValueMap_[var] = std::move(val);
if (var == "xxx") {
if (vals_.empty()) {
vals_.emplace_back(val);
indices_[var] = vals_.size() - 1;
} else {
vals_[indices_[var]] = val;
}
} else {
exprValueMap_[var] = std::move(val);
}
}

const Value& getInnerVar(const std::string& var) const override {
Expand Down Expand Up @@ -143,7 +152,7 @@ class ExpressionContextMock final : public ExpressionContext {

void setVar(const std::string& var, Value val) override {
// used by tests of list comprehesion, predicate or reduce
if (var == "n" || var == "p" || var == "totalNum") {
if (var == "n" || var == "p" || var == "totalNum" || var == "v") {
vals_.emplace_back(val);
indices_[var] = vals_.size() - 1;
}
Expand Down
32 changes: 23 additions & 9 deletions src/common/expression/test/ListComprehensionExpressionTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ TEST_F(ListComprehensionExpressionTest, ListComprehensionEvaluate) {
ASSERT_EQ(expected, value.getList());
}
{
// [n IN nodes(p) | n.age + 5]
// [n IN nodes(p) | n.player.age + 5]
auto v1 = Vertex("101", {Tag("player", {{"name", "joe"}, {"age", 18}})});
auto v2 = Vertex("102", {Tag("player", {{"name", "amber"}, {"age", 19}})});
auto v3 = Vertex("103", {Tag("player", {{"name", "shawdan"}, {"age", 20}})});
Expand All @@ -46,19 +46,25 @@ TEST_F(ListComprehensionExpressionTest, ListComprehensionEvaluate) {
path.steps.emplace_back(Step(v2, 1, "like", 0, {}));
path.steps.emplace_back(Step(v3, 1, "like", 0, {}));
gExpCtxt.setVar("p", path);

std::unordered_map<std::string, graph::AliasType> aliasTypeMap = {
{"xxx", graph::AliasType::kNode}};
ArgumentList *argList = ArgumentList::make(&pool);
argList->addArgument(VariableExpression::make(&pool, "p"));
auto expr = ListComprehensionExpression::make(
&pool,
"n",
"xxx",
FunctionCallExpression::make(&pool, "nodes", argList),
nullptr,
ArithmeticExpression::makeAdd(
&pool,
AttributeExpression::make(&pool,
VariableExpression::makeInner(&pool, "n"),
ConstantExpression::make(&pool, "age")),
graph::ExpressionUtils::rewriteAttr2LabelTagProp(
AttributeExpression::make(
&pool,
LabelAttributeExpression::make(&pool,
LabelExpression::make(&pool, "xxx"),
ConstantExpression::make(&pool, "player")),
ConstantExpression::make(&pool, "age")),
aliasTypeMap),
ConstantExpression::make(&pool, 5)));

auto value = Expression::eval(expr, gExpCtxt);
Expand Down Expand Up @@ -88,17 +94,25 @@ TEST_F(ListComprehensionExpressionTest, ListComprehensionExprToString) {
{
ArgumentList *argList = ArgumentList::make(&pool);
argList->addArgument(LabelExpression::make(&pool, "p"));
std::unordered_map<std::string, graph::AliasType> aliasTypeMap = {
{"n", graph::AliasType::kNode}};
auto expr = ListComprehensionExpression::make(
&pool,
"n",
FunctionCallExpression::make(&pool, "nodes", argList),
nullptr,
ArithmeticExpression::makeAdd(
&pool,
LabelAttributeExpression::make(
&pool, LabelExpression::make(&pool, "n"), ConstantExpression::make(&pool, "age")),
graph::ExpressionUtils::rewriteAttr2LabelTagProp(
AttributeExpression::make(
&pool,
LabelAttributeExpression::make(&pool,
LabelExpression::make(&pool, "n"),
ConstantExpression::make(&pool, "player")),
ConstantExpression::make(&pool, "age")),
aliasTypeMap),
ConstantExpression::make(&pool, 10)));
ASSERT_EQ("[n IN nodes(p) | (n.age+10)]", expr->toString());
ASSERT_EQ("[n IN nodes(p) | (n.player.age+10)]", expr->toString());
}
{
auto listItems = ExpressionList::make(&pool);
Expand Down
34 changes: 23 additions & 11 deletions src/common/expression/test/PredicateExpressionTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ TEST_F(PredicateExpressionTest, PredicateEvaluate) {
ASSERT_EQ(false, value.getBool());
}
{
// any(n IN nodes(p) WHERE n.age >= 19)
// any(xxx IN nodes(p) WHERE xxx.player.age >= 19)
auto v1 = Vertex("101", {Tag("player", {{"name", "joe"}, {"age", 18}})});
auto v2 = Vertex("102", {Tag("player", {{"name", "amber"}, {"age", 19}})});
auto v3 = Vertex("103", {Tag("player", {{"name", "shawdan"}, {"age", 20}})});
Expand All @@ -40,19 +40,25 @@ TEST_F(PredicateExpressionTest, PredicateEvaluate) {
path.steps.emplace_back(Step(v2, 1, "like", 0, {}));
path.steps.emplace_back(Step(v3, 1, "like", 0, {}));
gExpCtxt.setVar("p", path);

std::unordered_map<std::string, graph::AliasType> aliasTypeMap = {
{"xxx", graph::AliasType::kNode}};
ArgumentList *argList = ArgumentList::make(&pool);
argList->addArgument(VariableExpression::make(&pool, "p"));
auto expr = PredicateExpression::make(
&pool,
"any",
"n",
"xxx",
FunctionCallExpression::make(&pool, "nodes", argList),
RelationalExpression::makeGE(
&pool,
AttributeExpression::make(&pool,
VariableExpression::makeInner(&pool, "n"),
ConstantExpression::make(&pool, "age")),
graph::ExpressionUtils::rewriteAttr2LabelTagProp(
AttributeExpression::make(
&pool,
LabelAttributeExpression::make(&pool,
LabelExpression::make(&pool, "xxx"),
ConstantExpression::make(&pool, "player")),
ConstantExpression::make(&pool, "age")),
aliasTypeMap),
ConstantExpression::make(&pool, 19)));

auto value = Expression::eval(expr, gExpCtxt);
Expand Down Expand Up @@ -90,19 +96,25 @@ TEST_F(PredicateExpressionTest, PredicateEvaluate) {
path.steps.emplace_back(Step(v2, 1, "like", 0, {}));
path.steps.emplace_back(Step(v3, 1, "like", 0, {}));
gExpCtxt.setVar("p", path);

std::unordered_map<std::string, graph::AliasType> aliasTypeMap = {
{"xxx", graph::AliasType::kNode}};
ArgumentList *argList = ArgumentList::make(&pool);
argList->addArgument(VariableExpression::make(&pool, "p"));
auto expr = PredicateExpression::make(
&pool,
"none",
"n",
"xxx",
FunctionCallExpression::make(&pool, "nodes", argList),
RelationalExpression::makeGE(
&pool,
AttributeExpression::make(&pool,
VariableExpression::makeInner(&pool, "n"),
ConstantExpression::make(&pool, "age")),
graph::ExpressionUtils::rewriteAttr2LabelTagProp(
AttributeExpression::make(
&pool,
LabelAttributeExpression::make(&pool,
LabelExpression::make(&pool, "xxx"),
ConstantExpression::make(&pool, "player")),
ConstantExpression::make(&pool, "age")),
aliasTypeMap),
ConstantExpression::make(&pool, 19)));

auto value = Expression::eval(expr, gExpCtxt);
Expand Down
3 changes: 2 additions & 1 deletion src/graph/validator/MatchValidator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,8 @@ Status MatchValidator::validateUnwind(const UnwindClause *unwindClause,
return Status::SemanticError("Expression in UNWIND must be aliased (use AS)");
}
unwindCtx.alias = unwindClause->alias();
unwindCtx.unwindExpr = unwindClause->expr()->clone();
unwindCtx.unwindExpr = ExpressionUtils::rewriteAttr2LabelTagProp(unwindClause->expr()->clone(),
unwindCtx.aliasesAvailable);
if (ExpressionUtils::hasAny(unwindCtx.unwindExpr, {Expression::Kind::kAggregate})) {
return Status::SemanticError("Can't use aggregating expressions in unwind clause, `%s'",
unwindCtx.unwindExpr->toString().c_str());
Expand Down
5 changes: 3 additions & 2 deletions tests/tck/features/expression/ListComprehension.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ Feature: ListComprehension
When executing query:
"""
MATCH p = (n:player{name:"LeBron James"})<-[:like]-(m)
RETURN [n IN nodes(p) WHERE n.name NOT STARTS WITH "LeBron" | n.age + 100] AS r
RETURN [n IN nodes(p) WHERE n.player.name
NOT STARTS WITH "LeBron" | n.player.age + 100] AS r
"""
Then the result should be, in any order:
| r |
Expand All @@ -66,7 +67,7 @@ Feature: ListComprehension
When executing query:
"""
MATCH p = (n:player{name:"LeBron James"})-[:like]->(m)
RETURN [n IN nodes(p) | n.age + 100] AS r
RETURN [n IN nodes(p) | n.player.age + 100] AS r
"""
Then the result should be, in any order:
| r |
Expand Down
8 changes: 5 additions & 3 deletions tests/tck/features/expression/Predicate.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,10 @@ Feature: Predicate
When executing query:
"""
MATCH p = (n:player{name:"LeBron James"})<-[:like]-(m)
RETURN nodes(p)[0].name AS n1, nodes(p)[1].name AS n2,
all(n IN nodes(p) WHERE n.name NOT STARTS WITH "D") AS b
RETURN
nodes(p)[0].player.name AS n1,
nodes(p)[1].player.name AS n2,
all(n IN nodes(p) WHERE n.player.name NOT STARTS WITH "D") AS b
"""
Then the result should be, in any order:
| n1 | n2 | b |
Expand All @@ -199,7 +201,7 @@ Feature: Predicate
When executing query:
"""
MATCH p = (n:player{name:"LeBron James"})-[:like]->(m)
RETURN single(n IN nodes(p) WHERE n.age > 40) AS b
RETURN single(n IN nodes(p) WHERE n.player.age > 40) AS b
"""
Then the result should be, in any order:
| b |
Expand Down
12 changes: 6 additions & 6 deletions tests/tck/features/expression/Reduce.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ Feature: Reduce
"""
MATCH p = (n:player{name:"LeBron James"})<-[:like]-(m)
RETURN
nodes(p)[0].age AS age1,
nodes(p)[1].age AS age2,
reduce(totalAge = 100, n IN nodes(p) | totalAge + n.age) AS r
nodes(p)[0].player.age AS age1,
nodes(p)[1].player.age AS age2,
reduce(totalAge = 100, n IN nodes(p) | totalAge + n.player.age) AS r
"""
Then the result should be, in any order:
| age1 | age2 | r |
Expand All @@ -55,9 +55,9 @@ Feature: Reduce
When executing query:
"""
MATCH p = (n:player{name:"LeBron James"})-[:like]->(m)
RETURN nodes(p)[0].age AS age1,
nodes(p)[1].age AS age2,
reduce(x = 10, n IN nodes(p) | n.age - x) AS x
RETURN nodes(p)[0].player.age AS age1,
nodes(p)[1].player.age AS age2,
reduce(x = 10, n IN nodes(p) | n.player.age - x) AS x
"""
Then the result should be, in any order:
| age1 | age2 | x |
Expand Down
Loading

0 comments on commit 8cfc0bc

Please sign in to comment.