Resumed the evaluation fo vertices in AttributeExpression (UTs includ…

…ed) (#5229)
vesoft-inc · Jan 10, 2023 · 59b3523 · 59b3523
1 parent 40258dc
commit 59b3523
Show file tree

Hide file tree

Showing 9 changed files with 58 additions and 146 deletions.
diff --git a/src/common/expression/AttributeExpression.cpp b/src/common/expression/AttributeExpression.cpp
@@ -23,51 +23,17 @@ 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_;
       }
-      /*
-       * 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;
-            }
-          }
+      for (auto &tag : lvalue.getVertex().tags) {
+        auto iter = tag.props.find(rvalue.getStr());
+        if (iter != tag.props.end()) {
+          return iter->second;
         }
       }
-      return Value::kNullUnknownProp;
+      return Value::kNullValue;
     }
     case Value::Type::EDGE: {
       DCHECK(!rvalue.getStr().empty());

diff --git a/src/common/expression/test/AttributeExpressionTest.cpp b/src/common/expression/test/AttributeExpressionTest.cpp
@@ -2,10 +2,7 @@
  *
  * 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 {
 
@@ -85,8 +82,6 @@ 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"},
@@ -96,50 +91,35 @@ TEST_F(AttributeExpressionTest, VertexAttribute) {
       {"Tug", "War"},
       {"Venus", "RocksShow"},
   };
-  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);
+  {
+    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);
     ASSERT_TRUE(value.isStr());
     ASSERT_EQ("Kintyre", value.getStr());
   }
   {
-    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);
+    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);
     ASSERT_TRUE(value.isStr());
     ASSERT_EQ("Bop", value.getStr());
   }
   {
-    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);
+    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);
     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, expContext);
+    auto value = Expression::eval(expr, gExpCtxt);
     ASSERT_TRUE(value.isStr());
     ASSERT_EQ("vid", value.getStr());
   }
@@ -154,7 +134,7 @@ TEST_F(AttributeExpressionTest, DateTimeAttribute) {
     auto *right = LabelExpression::make(&pool, "not exist attribute");
     auto expr = AttributeExpression::make(&pool, left, right);
     auto value = Expression::eval(expr, gExpCtxt);
-    ASSERT_EQ(Value::kNullUnknownProp, value);
+    ASSERT_EQ(Value::kNullValue, value);
   }
   {
     auto *left = ConstantExpression::make(&pool, Value(dt));
@@ -168,7 +148,7 @@ TEST_F(AttributeExpressionTest, DateTimeAttribute) {
     auto *right = LabelExpression::make(&pool, "not exist attribute");
     auto expr = AttributeExpression::make(&pool, left, right);
     auto value = Expression::eval(expr, gExpCtxt);
-    ASSERT_EQ(Value::kNullUnknownProp, value);
+    ASSERT_EQ(Value::kNullValue, value);
   }
   {
     auto *left = ConstantExpression::make(&pool, Value(d));
@@ -182,7 +162,7 @@ TEST_F(AttributeExpressionTest, DateTimeAttribute) {
     auto *right = LabelExpression::make(&pool, "not exist attribute");
     auto expr = AttributeExpression::make(&pool, left, right);
     auto value = Expression::eval(expr, gExpCtxt);
-    ASSERT_EQ(Value::kNullUnknownProp, value);
+    ASSERT_EQ(Value::kNullValue, value);
   }
   {
     auto *left = ConstantExpression::make(&pool, Value(t));

diff --git a/src/common/expression/test/ExpressionContextMock.h b/src/common/expression/test/ExpressionContextMock.h
@@ -24,16 +24,7 @@ class ExpressionContextMock final : public ExpressionContext {
   }
 
   void setInnerVar(const std::string& var, Value val) override {
-    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);
-    }
+    exprValueMap_[var] = std::move(val);
   }
 
   const Value& getInnerVar(const std::string& var) const override {
@@ -152,7 +143,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" || var == "v") {
+    if (var == "n" || var == "p" || var == "totalNum") {
       vals_.emplace_back(val);
       indices_[var] = vals_.size() - 1;
     }

diff --git a/src/common/expression/test/ListComprehensionExpressionTest.cpp b/src/common/expression/test/ListComprehensionExpressionTest.cpp
@@ -37,7 +37,7 @@ TEST_F(ListComprehensionExpressionTest, ListComprehensionEvaluate) {
     ASSERT_EQ(expected, value.getList());
   }
   {
-    // [n IN nodes(p) | n.player.age + 5]
+    // [n IN nodes(p) | n.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}})});
@@ -46,25 +46,19 @@ 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,
-        "xxx",
+        "n",
         FunctionCallExpression::make(&pool, "nodes", argList),
         nullptr,
         ArithmeticExpression::makeAdd(
             &pool,
-            graph::ExpressionUtils::rewriteAttr2LabelTagProp(
-                AttributeExpression::make(
-                    &pool,
-                    LabelAttributeExpression::make(&pool,
-                                                   LabelExpression::make(&pool, "xxx"),
-                                                   ConstantExpression::make(&pool, "player")),
-                    ConstantExpression::make(&pool, "age")),
-                aliasTypeMap),
+            AttributeExpression::make(&pool,
+                                      VariableExpression::makeInner(&pool, "n"),
+                                      ConstantExpression::make(&pool, "age")),
             ConstantExpression::make(&pool, 5)));
 
     auto value = Expression::eval(expr, gExpCtxt);
@@ -94,25 +88,17 @@ 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,
-            graph::ExpressionUtils::rewriteAttr2LabelTagProp(
-                AttributeExpression::make(
-                    &pool,
-                    LabelAttributeExpression::make(&pool,
-                                                   LabelExpression::make(&pool, "n"),
-                                                   ConstantExpression::make(&pool, "player")),
-                    ConstantExpression::make(&pool, "age")),
-                aliasTypeMap),
+            LabelAttributeExpression::make(
+                &pool, LabelExpression::make(&pool, "n"), ConstantExpression::make(&pool, "age")),
             ConstantExpression::make(&pool, 10)));
-    ASSERT_EQ("[n IN nodes(p) | (n.player.age+10)]", expr->toString());
+    ASSERT_EQ("[n IN nodes(p) | (n.age+10)]", expr->toString());
   }
   {
     auto listItems = ExpressionList::make(&pool);

diff --git a/src/common/expression/test/PredicateExpressionTest.cpp b/src/common/expression/test/PredicateExpressionTest.cpp
@@ -31,7 +31,7 @@ TEST_F(PredicateExpressionTest, PredicateEvaluate) {
     ASSERT_EQ(false, value.getBool());
   }
   {
-    // any(xxx IN nodes(p) WHERE xxx.player.age >= 19)
+    // any(n IN nodes(p) WHERE n.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}})});
@@ -40,25 +40,19 @@ 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",
-        "xxx",
+        "n",
         FunctionCallExpression::make(&pool, "nodes", argList),
         RelationalExpression::makeGE(
             &pool,
-            graph::ExpressionUtils::rewriteAttr2LabelTagProp(
-                AttributeExpression::make(
-                    &pool,
-                    LabelAttributeExpression::make(&pool,
-                                                   LabelExpression::make(&pool, "xxx"),
-                                                   ConstantExpression::make(&pool, "player")),
-                    ConstantExpression::make(&pool, "age")),
-                aliasTypeMap),
+            AttributeExpression::make(&pool,
+                                      VariableExpression::makeInner(&pool, "n"),
+                                      ConstantExpression::make(&pool, "age")),
             ConstantExpression::make(&pool, 19)));
 
     auto value = Expression::eval(expr, gExpCtxt);
@@ -96,25 +90,19 @@ 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",
-        "xxx",
+        "n",
         FunctionCallExpression::make(&pool, "nodes", argList),
         RelationalExpression::makeGE(
             &pool,
-            graph::ExpressionUtils::rewriteAttr2LabelTagProp(
-                AttributeExpression::make(
-                    &pool,
-                    LabelAttributeExpression::make(&pool,
-                                                   LabelExpression::make(&pool, "xxx"),
-                                                   ConstantExpression::make(&pool, "player")),
-                    ConstantExpression::make(&pool, "age")),
-                aliasTypeMap),
+            AttributeExpression::make(&pool,
+                                      VariableExpression::makeInner(&pool, "n"),
+                                      ConstantExpression::make(&pool, "age")),
             ConstantExpression::make(&pool, 19)));
 
     auto value = Expression::eval(expr, gExpCtxt);

diff --git a/tests/tck/features/expression/Attribute1.feature b/tests/tck/features/expression/Attribute1.feature
@@ -1,6 +1,7 @@
 # Copyright (c) 2021 vesoft inc. All rights reserved.
 #
 # This source code is licensed under Apache 2.0 License.
+@skip
 Feature: Attribute using test
 
   Background:

diff --git a/tests/tck/features/expression/ListComprehension.feature b/tests/tck/features/expression/ListComprehension.feature
@@ -53,8 +53,8 @@ Feature: ListComprehension
     When executing query:
       """
       MATCH p = (n:player{name:"LeBron James"})<-[:like]-(m)
-      RETURN [n IN nodes(p) WHERE n.player.name
-      NOT STARTS WITH "LeBron" | n.player.age + 100] AS r
+      RETURN [n IN nodes(p) WHERE n.name
+      NOT STARTS WITH "LeBron" | n.age + 100] AS r
       """
     Then the result should be, in any order:
       | r     |
@@ -67,7 +67,7 @@ Feature: ListComprehension
     When executing query:
       """
       MATCH p = (n:player{name:"LeBron James"})-[:like]->(m)
-      RETURN [n IN nodes(p) | n.player.age + 100] AS r
+      RETURN [n IN nodes(p) | n.age + 100] AS r
       """
     Then the result should be, in any order:
       | r          |

diff --git a/tests/tck/features/expression/Predicate.feature b/tests/tck/features/expression/Predicate.feature
@@ -285,9 +285,9 @@ Feature: Predicate
       """
       MATCH p = (n:player{name:"LeBron James"})<-[:like]-(m)
       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
+        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
       """
     Then the result should be, in any order:
       | n1             | n2                | b     |
@@ -300,7 +300,7 @@ Feature: Predicate
     When executing query:
       """
       MATCH p = (n:player{name:"LeBron James"})-[:like]->(m)
-      RETURN single(n IN nodes(p) WHERE n.player.age > 40) AS b
+      RETURN single(n IN nodes(p) WHERE n.age > 40) AS b
       """
     Then the result should be, in any order:
       | b    |