Enhance attribute-accessing expression to ensure self-consistency (#5230

) * Revert "Remove all UNKNOWN_PROP as a type of null. (#4907)" (#5149) This reverts commit aa62416. Co-authored-by: Sophie <[email protected]> * Enhance attribute-accessing expression to ensure self-consistency Fix tck Fix parser small delete Fix tck tck fmt fix ut fix ut Fix ut Fix tck Delete v.tag.prop check Fix tck Skip some tck cases related ngdata add test case Co-authored-by: Cheng Xuntao <[email protected]> Co-authored-by: Sophie <[email protected]>
vesoft-inc · Jan 28, 2023 · c054a97 · c054a97
1 parent 87ebe35
commit c054a97
Show file tree

Hide file tree

Showing 17 changed files with 448 additions and 192 deletions.
diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp
@@ -292,6 +292,14 @@ Value::Value(Duration&& v) {
   setDU(std::make_unique<Duration>(std::move(v)));
 }
 
+Value::Value(const std::unordered_map<std::string, Value>& map) {
+  setM(std::make_unique<Map>(map));
+}
+
+Value::Value(std::unordered_map<std::string, Value>&& map) {
+  setM(std::make_unique<Map>(std::move(map)));
+}
+
 const std::string& Value::typeName() const {
   static const std::unordered_map<Type, std::string> typeNames = {
       {Type::__EMPTY__, "__EMPTY__"},

diff --git a/src/common/datatypes/Value.h b/src/common/datatypes/Value.h
@@ -91,49 +91,51 @@ struct Value {
   // matched ctor it will convert to bool type and the match the bool value
   // ctor, So we disable all pointer ctor except the char*
   template <typename T>
-  Value(T*) = delete;                    // NOLINT
-  Value(const std::nullptr_t) = delete;  // NOLINT
-  Value(const NullType& v);              // NOLINT
-  Value(NullType&& v);                   // NOLINT
-  Value(const bool& v);                  // NOLINT
-  Value(bool&& v);                       // NOLINT
-  Value(const int8_t& v);                // NOLINT
-  Value(int8_t&& v);                     // NOLINT
-  Value(const int16_t& v);               // NOLINT
-  Value(int16_t&& v);                    // NOLINT
-  Value(const int32_t& v);               // NOLINT
-  Value(int32_t&& v);                    // NOLINT
-  Value(const int64_t& v);               // NOLINT
-  Value(int64_t&& v);                    // NOLINT
-  Value(const double& v);                // NOLINT
-  Value(double&& v);                     // NOLINT
-  Value(const std::string& v);           // NOLINT
-  Value(std::string&& v);                // NOLINT
-  Value(const char* v);                  // NOLINT
-  Value(const Date& v);                  // NOLINT
-  Value(Date&& v);                       // NOLINT
-  Value(const Time& v);                  // NOLINT
-  Value(Time&& v);                       // NOLINT
-  Value(const DateTime& v);              // NOLINT
-  Value(DateTime&& v);                   // NOLINT
-  Value(const Vertex& v);                // NOLINT
-  Value(Vertex&& v);                     // NOLINT
-  Value(const Edge& v);                  // NOLINT
-  Value(Edge&& v);                       // NOLINT
-  Value(const Path& v);                  // NOLINT
-  Value(Path&& v);                       // NOLINT
-  Value(const List& v);                  // NOLINT
-  Value(List&& v);                       // NOLINT
-  Value(const Map& v);                   // NOLINT
-  Value(Map&& v);                        // NOLINT
-  Value(const Set& v);                   // NOLINT
-  Value(Set&& v);                        // NOLINT
-  Value(const DataSet& v);               // NOLINT
-  Value(DataSet&& v);                    // NOLINT
-  Value(const Geography& v);             // NOLINT
-  Value(Geography&& v);                  // NOLINT
-  Value(const Duration& v);              // NOLINT
-  Value(Duration&& v);                   // NOLINT
+  Value(T*) = delete;                                        // NOLINT
+  Value(const std::nullptr_t) = delete;                      // NOLINT
+  Value(const NullType& v);                                  // NOLINT
+  Value(NullType&& v);                                       // NOLINT
+  Value(const bool& v);                                      // NOLINT
+  Value(bool&& v);                                           // NOLINT
+  Value(const int8_t& v);                                    // NOLINT
+  Value(int8_t&& v);                                         // NOLINT
+  Value(const int16_t& v);                                   // NOLINT
+  Value(int16_t&& v);                                        // NOLINT
+  Value(const int32_t& v);                                   // NOLINT
+  Value(int32_t&& v);                                        // NOLINT
+  Value(const int64_t& v);                                   // NOLINT
+  Value(int64_t&& v);                                        // NOLINT
+  Value(const double& v);                                    // NOLINT
+  Value(double&& v);                                         // NOLINT
+  Value(const std::string& v);                               // NOLINT
+  Value(std::string&& v);                                    // NOLINT
+  Value(const char* v);                                      // NOLINT
+  Value(const Date& v);                                      // NOLINT
+  Value(Date&& v);                                           // NOLINT
+  Value(const Time& v);                                      // NOLINT
+  Value(Time&& v);                                           // NOLINT
+  Value(const DateTime& v);                                  // NOLINT
+  Value(DateTime&& v);                                       // NOLINT
+  Value(const Vertex& v);                                    // NOLINT
+  Value(Vertex&& v);                                         // NOLINT
+  Value(const Edge& v);                                      // NOLINT
+  Value(Edge&& v);                                           // NOLINT
+  Value(const Path& v);                                      // NOLINT
+  Value(Path&& v);                                           // NOLINT
+  Value(const List& v);                                      // NOLINT
+  Value(List&& v);                                           // NOLINT
+  Value(const Map& v);                                       // NOLINT
+  Value(Map&& v);                                            // NOLINT
+  Value(const Set& v);                                       // NOLINT
+  Value(Set&& v);                                            // NOLINT
+  Value(const DataSet& v);                                   // NOLINT
+  Value(DataSet&& v);                                        // NOLINT
+  Value(const Geography& v);                                 // NOLINT
+  Value(Geography&& v);                                      // NOLINT
+  Value(const Duration& v);                                  // NOLINT
+  Value(Duration&& v);                                       // NOLINT
+  Value(const std::unordered_map<std::string, Value>& map);  // NOLINT
+  Value(std::unordered_map<std::string, Value>&& map);       // NOLINT
   ~Value() {
     clear();
   }

diff --git a/src/common/expression/AttributeExpression.cpp b/src/common/expression/AttributeExpression.cpp
@@ -17,23 +17,30 @@ const Value &AttributeExpression::eval(ExpressionContext &ctx) {
   auto &lvalue = left()->eval(ctx);
   auto &rvalue = right()->eval(ctx);
   DCHECK(rvalue.isStr());
+  auto la = [&rvalue](const Tag &t) { return t.name == rvalue.getStr(); };
 
   // TODO(dutor) Take care of the builtin properties, _src, _vid, _type, etc.
   switch (lvalue.type()) {
-    case Value::Type::MAP:
-      return lvalue.getMap().at(rvalue.getStr());
+    case Value::Type::MAP: {
+      auto &m = lvalue.getMap().kvs;
+      auto iter = m.find(rvalue.getStr());
+      if (iter == m.end()) {
+        return Value::kNullValue;
+      }
+      return iter->second;
+    }
     case Value::Type::VERTEX: {
       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;
-        }
+      const auto &tags = lvalue.getVertex().tags;
+      auto iter = std::find_if(tags.begin(), tags.end(), la);
+      if (iter == tags.end()) {
+        return Value::kNullValue;
       }
-      return Value::kNullValue;
+      result_.setMap(Map(iter->props));
+      return result_;
     }
     case Value::Type::EDGE: {
       DCHECK(!rvalue.getStr().empty());
@@ -64,11 +71,14 @@ const Value &AttributeExpression::eval(ExpressionContext &ctx) {
     case Value::Type::DATETIME:
       result_ = time::TimeUtils::getDateTimeAttr(lvalue.getDateTime(), rvalue.getStr());
       return result_;
-    default:
-      if (lvalue.isNull() && lvalue.getNull() == NullType::UNKNOWN_PROP) {
-        // return UNKNOWN_PROP as plain null values, instead of bad type.
-        return Value::kNullValue;
+    case Value::Type::NULLVALUE: {
+      if (lvalue.isBadNull()) {
+        return Value::kNullBadType;
       }
+      return Value::kNullValue;
+    }
+    default:
+      // Unexpected data types
       return Value::kNullBadType;
   }
 }

diff --git a/src/common/expression/test/AttributeExpressionTest.cpp b/src/common/expression/test/AttributeExpressionTest.cpp
@@ -82,6 +82,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"},
@@ -92,28 +94,38 @@ TEST_F(AttributeExpressionTest, VertexAttribute) {
       {"Venus", "RocksShow"},
   };
   {
-    auto *left = ConstantExpression::make(&pool, Value(vertex));
-    auto *right = LabelExpression::make(&pool, "Mull");
-    auto expr = AttributeExpression::make(&pool, left, right);
+    auto expr = AttributeExpression::make(
+        &pool,
+        AttributeExpression::make(&pool,
+                                  ConstantExpression::make(&pool, Value(vertex)),
+                                  ConstantExpression::make(&pool, "tag0")),
+        ConstantExpression::make(&pool, "Mull"));
+
     auto value = Expression::eval(expr, gExpCtxt);
     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 expr = AttributeExpression::make(
+        &pool,
+        AttributeExpression::make(&pool,
+                                  ConstantExpression::make(&pool, Value(vertex)),
+                                  ConstantExpression::make(&pool, "tag1")),
+        ConstantExpression::make(&pool, "Bip"));
     auto value = Expression::eval(expr, gExpCtxt);
     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 expr = AttributeExpression::make(
+        &pool,
+        AttributeExpression::make(&pool,
+                                  ConstantExpression::make(&pool, Value(vertex)),
+                                  ConstantExpression::make(&pool, "tag2")),
+        ConstantExpression::make(&pool, "Venus"));
+
     auto value = Expression::eval(expr, gExpCtxt);
-    ASSERT_TRUE(value.isStr());
-    ASSERT_EQ("Mars", value.getStr());
+    ASSERT_TRUE(value.isNull());
   }
   {
     auto *left = ConstantExpression::make(&pool, Value(vertex));
@@ -134,7 +146,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::kNullValue, value);
+    ASSERT_EQ(Value::kNullUnknownProp, value);
   }
   {
     auto *left = ConstantExpression::make(&pool, Value(dt));
@@ -148,7 +160,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::kNullValue, value);
+    ASSERT_EQ(Value::kNullUnknownProp, value);
   }
   {
     auto *left = ConstantExpression::make(&pool, Value(d));
@@ -162,7 +174,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::kNullValue, value);
+    ASSERT_EQ(Value::kNullUnknownProp, value);
   }
   {
     auto *left = ConstantExpression::make(&pool, Value(t));

diff --git a/src/common/expression/test/ListComprehensionExpressionTest.cpp b/src/common/expression/test/ListComprehensionExpressionTest.cpp
@@ -56,9 +56,12 @@ TEST_F(ListComprehensionExpressionTest, ListComprehensionEvaluate) {
         nullptr,
         ArithmeticExpression::makeAdd(
             &pool,
-            AttributeExpression::make(&pool,
-                                      VariableExpression::makeInner(&pool, "n"),
-                                      ConstantExpression::make(&pool, "age")),
+            AttributeExpression::make(
+                &pool,
+                AttributeExpression::make(&pool,
+                                          VariableExpression::makeInner(&pool, "n"),
+                                          ConstantExpression::make(&pool, "player")),
+                ConstantExpression::make(&pool, "age")),
             ConstantExpression::make(&pool, 5)));
 
     auto value = Expression::eval(expr, gExpCtxt);

diff --git a/src/common/expression/test/PredicateExpressionTest.cpp b/src/common/expression/test/PredicateExpressionTest.cpp
@@ -50,9 +50,12 @@ TEST_F(PredicateExpressionTest, PredicateEvaluate) {
         FunctionCallExpression::make(&pool, "nodes", argList),
         RelationalExpression::makeGE(
             &pool,
-            AttributeExpression::make(&pool,
-                                      VariableExpression::makeInner(&pool, "n"),
-                                      ConstantExpression::make(&pool, "age")),
+            AttributeExpression::make(
+                &pool,
+                AttributeExpression::make(&pool,
+                                          VariableExpression::makeInner(&pool, "n"),
+                                          ConstantExpression::make(&pool, "player")),
+                ConstantExpression::make(&pool, "age")),
             ConstantExpression::make(&pool, 19)));
 
     auto value = Expression::eval(expr, gExpCtxt);
@@ -100,9 +103,12 @@ TEST_F(PredicateExpressionTest, PredicateEvaluate) {
         FunctionCallExpression::make(&pool, "nodes", argList),
         RelationalExpression::makeGE(
             &pool,
-            AttributeExpression::make(&pool,
-                                      VariableExpression::makeInner(&pool, "n"),
-                                      ConstantExpression::make(&pool, "age")),
+            AttributeExpression::make(
+                &pool,
+                AttributeExpression::make(&pool,
+                                          VariableExpression::makeInner(&pool, "n"),
+                                          ConstantExpression::make(&pool, "player")),
+                ConstantExpression::make(&pool, "age")),
             ConstantExpression::make(&pool, 19)));
 
     auto value = Expression::eval(expr, gExpCtxt);

diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp
@@ -982,11 +982,6 @@ Status MatchValidator::checkAlias(
       auto name = static_cast<const LabelAttributeExpression *>(refExpr)->left()->name();
       auto res = getAliasType(aliasesAvailable, name);
       NG_RETURN_IF_ERROR(res);
-      if (res.value() == AliasType::kNode) {
-        return Status::SemanticError(
-            "To get the property of the vertex in `%s', should use the format `var.tag.prop'",
-            refExpr->toString().c_str());
-      }
       return Status::OK();
     }
     case Expression::Kind::kEdgeSrc: {

diff --git a/tests/tck/features/expression/Attribute.feature b/tests/tck/features/expression/Attribute.feature
@@ -61,8 +61,8 @@ Feature: Attribute
       RETURN {k1 : 1, k2: true}.K1 AS k
       """
     Then the result should be, in any order:
-      | k            |
-      | UNKNOWN_PROP |
+      | k    |
+      | NULL |
     When executing query:
       """
       MATCH (v) WHERE id(v) == 'Tim Duncan' RETURN v.player.name
@@ -122,7 +122,7 @@ Feature: Attribute
       """
     Then the result should be, in any order:
       | not_exists_attr |
-      | UNKNOWN_PROP    |
+      | NULL            |
     When executing query:
       """
       MATCH (v) WHERE id(v) == 'Tim Duncan' RETURN v.player.not_exists_attr

diff --git a/tests/tck/features/expression/Attribute1.feature b/tests/tck/features/expression/Attribute1.feature
@@ -1,7 +1,6 @@
 # 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.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     |
@@ -67,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          |

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].name AS n1,
-        nodes(p)[1].name AS n2,
-        all(n IN nodes(p) WHERE n.name NOT STARTS WITH "D") AS b
+        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     |
@@ -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.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    |