Property storage now supports Map

Summary:
Added:
- map support in PropertyValue
- conversion of map TypedValue to PropertyValue if appropriate flag is set (undocumented because it's private)
- ordering of map PropertyValue in LabelPropertyIndex
- issue raised regarding list and value property modifications in storage (currently unsupported)

Maybe I missed some feature or whatever?

Reviewers: mislav.bradac, buda, teon.banek

Reviewed By: mislav.bradac, buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D692

CHANGELOG.md

@@ -4,8 +4,9 @@
 
 ### Major Features and Improvements
 
-* CASE construct (without aggregations)
+* CASE construct (without aggregations).
-* rand() function added
+* `rand` function added.
+* Maps can now be stored as vertex/edge properties.
 
 ### Bug Fixes and Other Changes
 

docs/user_technical/data-types.md

@@ -34,11 +34,14 @@ types. Following is a table of supported data types.
  `Integer` | An integer number.
  `Float`   | A floating-point number, i.e. a real number.
  `List`    | A list containing any number of property values of any supported type. It can be used to store multiple values under a single property name.
+ `Map`     | A mapping of string keys to values of any supported type.
 
-Note that even though map literals are supported in openCypher queries, they can't be stored in the graph. For example, the following query is legal:
+ Note that even though it's possible to store `List` and `Map` property values, it is not possible to modify them. It is however possible to replace them completely. So, the following queries are legal:
 
-     MATCH (n:Person) RETURN {name: n.name, age: n.age}
+    CREATE (:Node {property: [1, 2, 3]})
+    CREATE (:Node {property: {key: "value"}})
 
-However, the next query is not:
+However, these queries are not:
 
-     MATCH (n:Person {name: "John"}) SET n.address = {city: "London", street: "Fleet St."}
+    MATCH (n:Node) SET n.property[0] = 0
+    MATCH (n:Node) SET n.property.key = "other value"

src/database/indexes/label_property_index.hpp

@@ -446,6 +446,19 @@ class LabelPropertyIndex {
             return lexicographical_compare(va.begin(), va.end(), vb.begin(),
                                            vb.end(), Less);
           }
+          case PropertyValue::Type::Map: {
+            auto ma = a.Value<std::map<std::string, PropertyValue>>();
+            auto mb = b.Value<std::map<std::string, PropertyValue>>();
+            if (ma.size() != mb.size()) return ma.size() < mb.size();
+            const auto cmp = [](const auto &a, const auto &b) {
+              if (a.first != b.first)
+                return a.first < b.first;
+              else
+                return Less(a.second, b.second);
+            };
+            return lexicographical_compare(ma.begin(), ma.end(), mb.begin(),
+                                           mb.end(), cmp);
+          }
           default:
             permanent_fail("Unimplemented type operator.");
         }
@@ -482,12 +495,13 @@ class LabelPropertyIndex {
   };
 
   /**
-   * @brief - Insert value, vlist, vertex into corresponding index (key) if the
+   * @brief - Insert value, vlist, vertex into corresponding index (key) if
-   * index exists.
+   * the index exists.
    * @param index - into which index to add
    * @param value - value which to add
    * @param vlist - pointer to vlist entry to add
-   * @param vertex - pointer to vertex record entry to add (contained in vlist)
+   * @param vertex - pointer to vertex record entry to add (contained in
+   * vlist)
    */
   void Insert(SkipList<IndexEntry> &index, const PropertyValue &value,
               mvcc::VersionList<Vertex> *const vlist,

src/query/plan/operator.cpp

@@ -1202,6 +1202,12 @@ bool SetProperty::SetPropertyCursor::Pull(Frame &frame,
     case TypedValue::Type::Null:
       // Skip setting properties on Null (can occur in optional match).
       break;
+    case TypedValue::Type::Map:
+      // Semantically modifying a map makes sense, but it's not supported due to
+      // all the copying we do (when PropertyValue -> TypedValue and in
+      // ExpressionEvaluator). So even though we set a map property here, that
+      // is never visible to the user and it's not stored.
+      // TODO: fix above described bug
     default:
       throw QueryRuntimeException(
           "Properties can only be set on Vertices and Edges");

src/query/typed_value.cpp

@@ -33,11 +33,18 @@ TypedValue::TypedValue(const PropertyValue &value) {
       type_ = Type::String;
       new (&string_v) std::string(value.Value<std::string>());
       return;
-    case PropertyValue::Type::List:
+    case PropertyValue::Type::List: {
       type_ = Type::List;
       auto vec = value.Value<std::vector<PropertyValue>>();
       new (&list_v) std::vector<TypedValue>(vec.begin(), vec.end());
       return;
+    }
+    case PropertyValue::Type::Map: {
+      type_ = Type::Map;
+      auto map = value.Value<std::map<std::string, PropertyValue>>();
+      new (&map_v) std::map<std::string, TypedValue>(map.begin(), map.end());
+      return;
+    }
   }
   permanent_fail("Unsupported type");
 }
@@ -57,10 +64,14 @@ TypedValue::operator PropertyValue() const {
     case TypedValue::Type::List:
       return PropertyValue(
           std::vector<PropertyValue>(list_v.begin(), list_v.end()));
+    case TypedValue::Type::Map:
+      return PropertyValue(
+          std::map<std::string, PropertyValue>(map_v.begin(), map_v.end()));
     default:
-      throw TypedValueException(
+      break;
-          "Unsupported conversion from TypedValue to PropertyValue");
   }
+  throw TypedValueException(
+      "Unsupported conversion from TypedValue to PropertyValue");
 }
 
 // TODO: Refactor this. Value<bool> should be ValueBool. If we do it in that way
@@ -190,7 +201,6 @@ const std::vector<TypedValue> &TypedValue::ValueList() const {
   return Value<std::vector<TypedValue>>();
 }
 
-
 template <>
 std::map<std::string, TypedValue>
     &TypedValue::Value<std::map<std::string, TypedValue>>() {
@@ -273,9 +283,10 @@ bool TypedValue::IsPropertyValue() const {
     case Type::Double:
     case Type::String:
     case Type::List:
+    case Type::Map:
       return true;
     default:
-    return false;
+      return false;
   }
 }
 

src/storage/property_value.cpp

@@ -49,7 +49,16 @@ std::vector<PropertyValue> PropertyValue::Value<std::vector<PropertyValue>>()
   return *list_v;
 }
 
-PropertyValue::PropertyValue(const PropertyValue& other) : type_(other.type_) {
+template <>
+std::map<std::string, PropertyValue>
+PropertyValue::Value<std::map<std::string, PropertyValue>>() const {
+  if (type_ != PropertyValue::Type::Map) {
+    throw PropertyValueException("Incompatible template param and type");
+  }
+  return *map_v;
+}
+
+PropertyValue::PropertyValue(const PropertyValue &other) : type_(other.type_) {
   switch (other.type_) {
     case PropertyValue::Type::Null:
       return;
@@ -70,15 +79,20 @@ PropertyValue::PropertyValue(const PropertyValue& other) : type_(other.type_) {
       this->double_v = other.double_v;
       return;
 
-    case PropertyValue::Type::List:
+    case Type::List:
       new (&list_v) std::shared_ptr<std::vector<PropertyValue>>(other.list_v);
       return;
+
+    case Type::Map:
+      new (&map_v)
+          std::shared_ptr<std::map<std::string, PropertyValue>>(other.map_v);
+      return;
   }
 
   permanent_fail("Unsupported PropertyValue::Type");
 }
 
-std::ostream& operator<<(std::ostream& os, const PropertyValue::Type type) {
+std::ostream &operator<<(std::ostream &os, const PropertyValue::Type type) {
   switch (type) {
     case PropertyValue::Type::Null:
       return os << "null";
@@ -92,11 +106,13 @@ std::ostream& operator<<(std::ostream& os, const PropertyValue::Type type) {
       return os << "double";
     case PropertyValue::Type::List:
       return os << "list";
+    case PropertyValue::Type::Map:
+      return os << "map";
   }
   permanent_fail("Unsupported PropertyValue::Type");
 }
 
-std::ostream& operator<<(std::ostream& os, const PropertyValue& value) {
+std::ostream &operator<<(std::ostream &os, const PropertyValue &value) {
   switch (value.type_) {
     case PropertyValue::Type::Null:
       return os << "Null";
@@ -110,15 +126,22 @@ std::ostream& operator<<(std::ostream& os, const PropertyValue& value) {
       return os << value.Value<double>();
     case PropertyValue::Type::List:
       os << "[";
-      for (const auto& x : value.Value<std::vector<PropertyValue>>()) {
+      for (const auto &x : value.Value<std::vector<PropertyValue>>()) {
         os << x << ",";
       }
       return os << "]";
+    case PropertyValue::Type::Map:
+      os << "{";
+      for (const auto &kv :
+           value.Value<std::map<std::string, PropertyValue>>()) {
+        os << kv.first << ": " << kv.second << ",";
+      }
+      return os << "}";
   }
   permanent_fail("Unsupported PropertyValue::Type");
 }
 
-PropertyValue& PropertyValue::operator=(const PropertyValue& other) {
+PropertyValue &PropertyValue::operator=(const PropertyValue &other) {
   this->~PropertyValue();
   type_ = other.type_;
 
@@ -140,6 +163,10 @@ PropertyValue& PropertyValue::operator=(const PropertyValue& other) {
       case PropertyValue::Type::List:
         new (&list_v) std::shared_ptr<std::vector<PropertyValue>>(other.list_v);
         return *this;
+      case PropertyValue::Type::Map:
+        new (&map_v)
+            std::shared_ptr<std::map<std::string, PropertyValue>>(other.map_v);
+        return *this;
     }
   }
   permanent_fail("Unsupported PropertyValue::Type");
@@ -163,6 +190,9 @@ PropertyValue::~PropertyValue() {
     case Type::List:
       list_v.~shared_ptr<std::vector<PropertyValue>>();
       return;
+    case Type::Map:
+      map_v.~shared_ptr<std::map<std::string, PropertyValue>>();
+      return;
   }
   permanent_fail("Unsupported PropertyValue::Type");
 }

src/storage/property_value.hpp

@@ -2,6 +2,7 @@
 
 #include <cassert>
 #include <iostream>
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -23,7 +24,7 @@ class PropertyValue {
 
  public:
   /** A value type. Each type corresponds to exactly one C++ type */
-  enum class Type : unsigned { Null, String, Bool, Int, Double, List };
+  enum class Type : unsigned { Null, String, Bool, Int, Double, List, Map };
 
   // single static reference to Null, used whenever Null should be returned
   static const PropertyValue Null;
@@ -54,6 +55,11 @@ class PropertyValue {
     new (&list_v) std::shared_ptr<std::vector<PropertyValue>>(
         new std::vector<PropertyValue>(value));
   }
+  PropertyValue(const std::map<std::string, PropertyValue> &value)
+      : type_(Type::Map) {
+    new (&map_v) std::shared_ptr<std::map<std::string, PropertyValue>>(
+        new std::map<std::string, PropertyValue>(value));
+  }
 
   // assignment op
   PropertyValue &operator=(const PropertyValue &other);
@@ -86,6 +92,7 @@ class PropertyValue {
     // We support lists of values of different types, neo4j supports lists of
     // values of the same type.
     std::shared_ptr<std::vector<PropertyValue>> list_v;
+    std::shared_ptr<std::map<std::string, PropertyValue>> map_v;
   };
 
   /**

tests/qa/tck_engine/tests/memgraph_V1/features/map_operations.feature

@@ -10,12 +10,19 @@ Feature: Map operators
             | {a: 1, b: 'bla', c: [1, 2], d: {a: 42}} |
 
 
-  Scenario: Storing a map property fails
+  Scenario: Storing a map property
+        Given an empty graph
+        And having executed
+            """
+            CREATE ({prop: {x: 1, y: true, z: "bla"}})
+            """
         When executing query:
             """
-            CREATE (n {property: {a: 1, b: 2}})
+            MATCH (n) RETURN n.prop
             """
-	Then an error should be raised
+        Then the result should be:
+            | n.prop                    |
+            | {x: 1, y: true, z: 'bla'} |
 
 
   Scenario: A property lookup on a map literal

tests/unit/graph_db_accessor_index_api.cpp

@@ -286,8 +286,25 @@ TEST_F(GraphDbAccessorIndex, LabelPropertyValueSorting) {
     expected_property_value[40 + i] = vertex_accessor.PropsAt(property);
   }
 
+  // Maps. Declare a vector in the expected order, then shuffle when setting on
+  // vertices.
+  std::vector<std::map<std::string, PropertyValue>> maps{
+      {{"b", 12}},
+      {{"b", 12}, {"a", 77}},
+      {{"a", 77}, {"c", 0}},
+      {{"a", 78}, {"b", 12}}};
+  expected_property_value.insert(expected_property_value.end(), maps.begin(),
+                                 maps.end());
+  auto shuffled = maps;
+  std::random_shuffle(shuffled.begin(), shuffled.end());
+  for (const auto &map : shuffled) {
+    auto vertex_accessor = dba->InsertVertex();
+    vertex_accessor.add_label(label);
+    vertex_accessor.PropsSet(property, map);
+  }
+
   EXPECT_EQ(Count(dba->Vertices(label, property, false)), 0);
-  EXPECT_EQ(Count(dba->Vertices(label, property, true)), 50);
+  EXPECT_EQ(Count(dba->Vertices(label, property, true)), 54);
 
   int cnt = 0;
   for (auto vertex : dba->Vertices(label, property, true)) {
@@ -321,6 +338,20 @@ TEST_F(GraphDbAccessorIndex, LabelPropertyValueSorting) {
                   expected_value[0].Value<int64_t>());
         break;
       }
+      case PropertyValue::Type::Map: {
+        auto received_value =
+            property_value.Value<std::map<std::string, PropertyValue>>();
+        auto expected_value =
+            expected_property_value[cnt]
+                .Value<std::map<std::string, PropertyValue>>();
+        EXPECT_EQ(received_value.size(), expected_value.size());
+        for (const auto &kv : expected_value) {
+          auto found = expected_value.find(kv.first);
+          EXPECT_NE(found, expected_value.end());
+          EXPECT_EQ(kv.second.Value<int64_t>(), found->second.Value<int64_t>());
+        }
+        break;
+      }
       case PropertyValue::Type::Null:
         ASSERT_FALSE("Invalid value type.");
     }

tests/unit/property_value_store.cpp

@@ -6,8 +6,8 @@
 
 #include "gtest/gtest.h"
 
-#include "storage/property_value_store.hpp"
 #include "storage/property_value.hpp"
+#include "storage/property_value_store.hpp"
 
 using std::string;
 
@@ -108,8 +108,9 @@ TEST(PropertyValueStore, Accept) {
   int count_props = 0;
   int count_finish = 0;
 
-  auto handler =
+  auto handler = [&](const uint32_t, const PropertyValue &) {
-      [&](const uint32_t, const PropertyValue&) { count_props += 1; };
+    count_props += 1;
+  };
   auto finish = [&]() { count_finish += 1; };
 
   PropertyValueStore<uint32_t> props;
@@ -148,3 +149,23 @@ TEST(PropertyValueStore, InsertRetrieveList) {
   EXPECT_EQ(l[3].Value<std::string>(), "something");
   EXPECT_EQ(l[4].type(), PropertyValue::Type::Null);
 }
+
+TEST(PropertyValueStore, InsertRetrieveMap) {
+  PropertyValueStore<> props;
+  props.set(0, std::map<std::string, PropertyValue>{
+                   {"a", 1}, {"b", true}, {"c", "something"}});
+
+  auto p = props.at(0);
+  EXPECT_EQ(p.type(), PropertyValue::Type::Map);
+  auto m = p.Value<std::map<std::string, PropertyValue>>();
+  EXPECT_EQ(m.size(), 3);
+  auto get = [&m](const std::string &prop_name) {
+    return m.find(prop_name)->second;
+  };
+  EXPECT_EQ(get("a").type(), PropertyValue::Type::Int);
+  EXPECT_EQ(get("a").Value<int64_t>(), 1);
+  EXPECT_EQ(get("b").type(), PropertyValue::Type::Bool);
+  EXPECT_EQ(get("b").Value<bool>(), true);
+  EXPECT_EQ(get("c").type(), PropertyValue::Type::String);
+  EXPECT_EQ(get("c").Value<std::string>(), "something");
+}