Remove PropertyValueStore templatization

Summary:
A PropertyValueStore is not a generic data structure, but only ever used
to store properties in a Vertex/Edge. It has behaviours specific to it.
So, the templatization was not necessary.

Reviewers: buda

Reviewed By: buda

Subscribers: pullbot

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

src/storage/edge.hpp

@@ -22,7 +22,7 @@ class Edge : public mvcc::Record<Edge> {
   VertexAddress from_;
   VertexAddress to_;
   GraphDbTypes::EdgeType edge_type_;
-  PropertyValueStore<GraphDbTypes::Property> properties_;
+  PropertyValueStore properties_;
 
  private:
   Edge(const Edge &other)

src/storage/property_value_store.hpp

@@ -4,21 +4,19 @@
 #include <map>
 #include <vector>
 
+#include "database/graph_db_datatypes.hpp"
 #include "property_value.hpp"
 
 /**
  * A collection of properties accessed in a map-like way
- * using a key of type Properties::TKey.
+ * using a key of type Properties::Property.
  *
  * The underlying implementation is not necessarily std::map.
- *
-  * @tparam TKey The type of key used in this value store.
  */
-template <typename TKey = uint32_t>
 class PropertyValueStore {
- public:
+  using Property = GraphDbTypes::Property;
-  using sptr = std::shared_ptr<PropertyValueStore>;
 
+ public:
   /**
    * Returns a PropertyValue (by reference) at the given key.
    * If the key does not exist, the Null property is returned.
@@ -29,7 +27,7 @@ class PropertyValueStore {
    * @param key The key for which a PropertyValue is sought.
    * @return  See above.
    */
-  const PropertyValue &at(const TKey &key) const {
+  const PropertyValue &at(const Property &key) const {
     for (const auto &kv : props_)
       if (kv.first == key) return kv.second;
 
@@ -47,7 +45,7 @@ class PropertyValueStore {
    * @param value  The value to set.
    */
   template <typename TValue>
-  void set(const TKey &key, const TValue &value) {
+  void set(const Property &key, const TValue &value) {
     for (auto &kv : props_)
       if (kv.first == key) {
         kv.second = PropertyValue(value);
@@ -64,13 +62,15 @@ class PropertyValueStore {
    * to std::string, otherwise templating might cast the pointer
    * to something else (bool) and mess things up.
    */
-  void set(const TKey &key, const char *value) { set(key, std::string(value)); }
+  void set(const Property &key, const char *value) {
+    set(key, std::string(value));
+  }
 
   /**
    * Set overriding for PropertyValue. When setting a Null value it
    * calls 'erase' instead of inserting the Null into storage.
    */
-  void set(const TKey &key, const PropertyValue &value) {
+  void set(const Property &key, const PropertyValue &value) {
     if (value.type() == PropertyValue::Type::Null) {
       erase(key);
       return;
@@ -91,10 +91,11 @@ class PropertyValueStore {
    * @param key The key for which to remove the property.
    * @return  The number of removed properties (0 or 1).
    */
-  size_t erase(const TKey &key) {
+  size_t erase(const Property &key) {
-    auto found = std::find_if(
+    auto found = std::find_if(props_.begin(), props_.end(),
-        props_.begin(), props_.end(),
+                              [&key](std::pair<Property, PropertyValue> &kv) {
-        [&key](std::pair<TKey, PropertyValue> &kv) { return kv.first == key; });
+                                return kv.first == key;
+                              });
 
     if (found != props_.end()) {
       props_.erase(found);
@@ -103,30 +104,13 @@ class PropertyValueStore {
     return 0;
   }
 
-  /**
+  /** Removes all the properties from this store. */
-   * Removes all the properties from this store.
-   */
   void clear() { props_.clear(); }
 
-  /**
-   * @return The number of Properties in this collection.
-   */
   size_t size() const { return props_.size(); }
-
-  /**
-   * Returns a const iterator over key-value pairs.
-   *
-   * @return See above.
-   */
   auto begin() const { return props_.begin(); }
-
-  /**
-   * Returns an end iterator.
-   *
-   * @return See above.
-   */
   auto end() const { return props_.end(); }
 
  private:
-  std::vector<std::pair<TKey, PropertyValue>> props_;
+  std::vector<std::pair<Property, PropertyValue>> props_;
 };

src/storage/record_accessor.cpp

@@ -85,8 +85,7 @@ void RecordAccessor<Edge>::PropsClear() {
 }
 
 template <typename TRecord>
-const PropertyValueStore<GraphDbTypes::Property>
+const PropertyValueStore &RecordAccessor<TRecord>::Properties() const {
-    &RecordAccessor<TRecord>::Properties() const {
   return current().properties_;
 }
 

src/storage/record_accessor.hpp

@@ -75,7 +75,7 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
   void PropsClear();
 
   /** Returns the properties of this record. */
-  const PropertyValueStore<GraphDbTypes::Property> &Properties() const;
+  const PropertyValueStore &Properties() const;
 
   bool operator==(const RecordAccessor &other) const;
 

src/storage/vertex.hpp

@@ -17,7 +17,7 @@ class Vertex : public mvcc::Record<Vertex> {
   Edges out_;
   Edges in_;
   std::vector<GraphDbTypes::Label> labels_;
-  PropertyValueStore<GraphDbTypes::Property> properties_;
+  PropertyValueStore properties_;
 
  private:
   Vertex(const Vertex &other)

tests/unit/property_value_store.cpp

@@ -1,7 +1,3 @@
-//
-// Copyright 2017 Memgraph
-// Created by Florijan Stamenkovic on 24.01.17..
-//
 #include <vector>
 
 #include "gtest/gtest.h"
@@ -11,104 +7,109 @@
 
 using std::string;
 
-TEST(PropertyValueStore, At) {
+class PropertyValueStoreTest : public ::testing::Test {
+ protected:
+  PropertyValueStore props_;
+
+  void Set(int key, PropertyValue value) {
+    props_.set(GraphDbTypes::Property(key), value);
+  }
+
+  PropertyValue At(int key) { return props_.at(GraphDbTypes::Property(key)); }
+
+  auto Erase(int key) { return props_.erase(GraphDbTypes::Property(key)); }
+};
+
+TEST_F(PropertyValueStoreTest, At) {
   std::string some_string = "something";
 
-  PropertyValueStore<> props;
+  EXPECT_EQ(PropertyValue(At(0)).type(), PropertyValue::Type::Null);
-  EXPECT_EQ(PropertyValue(props.at(0)).type(), PropertyValue::Type::Null);
+  Set(0, some_string);
-  props.set(0, some_string);
+  EXPECT_EQ(PropertyValue(At(0)).Value<string>(), some_string);
-  EXPECT_EQ(PropertyValue(props.at(0)).Value<string>(), some_string);
+  Set(120, 42);
-  props.set(120, 42);
+  EXPECT_EQ(PropertyValue(At(120)).Value<int64_t>(), 42);
-  EXPECT_EQ(PropertyValue(props.at(120)).Value<int64_t>(), 42);
 }
 
-TEST(PropertyValueStore, AtNull) {
+TEST_F(PropertyValueStoreTest, AtNull) {
-  PropertyValueStore<> props;
+  EXPECT_EQ(At(0).type(), PropertyValue::Type::Null);
-  EXPECT_EQ(props.at(0).type(), PropertyValue::Type::Null);
+  EXPECT_EQ(At(100).type(), PropertyValue::Type::Null);
-  EXPECT_EQ(props.at(100).type(), PropertyValue::Type::Null);
 
   // set one prop and test it's not null
-  props.set(0, true);
+  Set(0, true);
-  EXPECT_NE(props.at(0).type(), PropertyValue::Type::Null);
+  EXPECT_NE(At(0).type(), PropertyValue::Type::Null);
-  EXPECT_EQ(props.at(100).type(), PropertyValue::Type::Null);
+  EXPECT_EQ(At(100).type(), PropertyValue::Type::Null);
 }
 
-TEST(PropertyValueStore, SetNull) {
+TEST_F(PropertyValueStoreTest, SetNull) {
-  PropertyValueStore<> props;
+  Set(11, PropertyValue::Null);
-  props.set(11, PropertyValue::Null);
+  EXPECT_EQ(0, props_.size());
-  EXPECT_EQ(0, props.size());
 }
 
-TEST(PropertyValueStore, Remove) {
+TEST_F(PropertyValueStoreTest, Remove) {
   // set some props
-  PropertyValueStore<> props;
+  Set(11, "a");
-  props.set(11, "a");
+  Set(30, "b");
-  props.set(30, "b");
+  EXPECT_NE(At(11).type(), PropertyValue::Type::Null);
-  EXPECT_NE(props.at(11).type(), PropertyValue::Type::Null);
+  EXPECT_NE(At(30).type(), PropertyValue::Type::Null);
-  EXPECT_NE(props.at(30).type(), PropertyValue::Type::Null);
+  EXPECT_EQ(props_.size(), 2);
-  EXPECT_EQ(props.size(), 2);
 
-  props.erase(11);
+  Erase(11);
-  EXPECT_EQ(props.size(), 1);
+  EXPECT_EQ(props_.size(), 1);
-  EXPECT_EQ(props.at(11).type(), PropertyValue::Type::Null);
+  EXPECT_EQ(At(11).type(), PropertyValue::Type::Null);
 
-  EXPECT_EQ(props.erase(30), 1);
+  EXPECT_EQ(Erase(30), 1);
-  EXPECT_EQ(props.size(), 0);
+  EXPECT_EQ(props_.size(), 0);
-  EXPECT_EQ(props.at(30).type(), PropertyValue::Type::Null);
+  EXPECT_EQ(At(30).type(), PropertyValue::Type::Null);
 
-  EXPECT_EQ(props.erase(1000), 0);
+  EXPECT_EQ(Erase(1000), 0);
 }
 
-TEST(PropertyValueStore, Clear) {
+TEST_F(PropertyValueStoreTest, Clear) {
   // set some props
-  PropertyValueStore<> props;
+  EXPECT_EQ(props_.size(), 0);
-  EXPECT_EQ(props.size(), 0);
+  props_.clear();
-  props.clear();
 
-  EXPECT_EQ(props.size(), 0);
+  EXPECT_EQ(props_.size(), 0);
-  props.set(11, "a");
+  Set(11, "a");
-  props.set(30, "b");
+  Set(30, "b");
-  EXPECT_EQ(props.size(), 2);
+  EXPECT_EQ(props_.size(), 2);
-  props.clear();
+  props_.clear();
-  EXPECT_EQ(props.size(), 0);
+  EXPECT_EQ(props_.size(), 0);
 }
 
-TEST(PropertyValueStore, Replace) {
+TEST_F(PropertyValueStoreTest, Replace) {
-  PropertyValueStore<> props;
+  Set(10, 42);
-  props.set(10, 42);
+  EXPECT_EQ(At(10).Value<int64_t>(), 42);
-  EXPECT_EQ(props.at(10).Value<int64_t>(), 42);
+  Set(10, 0.25f);
-  props.set(10, 0.25f);
+  EXPECT_EQ(At(10).type(), PropertyValue::Type::Double);
-  EXPECT_EQ(props.at(10).type(), PropertyValue::Type::Double);
+  EXPECT_FLOAT_EQ(At(10).Value<double>(), 0.25);
-  EXPECT_FLOAT_EQ(props.at(10).Value<double>(), 0.25);
 }
 
-TEST(PropertyValueStore, Size) {
+TEST_F(PropertyValueStoreTest, Size) {
-  PropertyValueStore<> props;
+  EXPECT_EQ(props_.size(), 0);
-  EXPECT_EQ(props.size(), 0);
 
-  props.set(0, "something");
+  Set(0, "something");
-  EXPECT_EQ(props.size(), 1);
+  EXPECT_EQ(props_.size(), 1);
-  props.set(0, true);
+  Set(0, true);
-  EXPECT_EQ(props.size(), 1);
+  EXPECT_EQ(props_.size(), 1);
-  props.set(1, true);
+  Set(1, true);
-  EXPECT_EQ(props.size(), 2);
+  EXPECT_EQ(props_.size(), 2);
 
-  for (int i = 0; i < 100; ++i) props.set(i + 20, true);
+  for (int i = 0; i < 100; ++i) Set(i + 20, true);
-  EXPECT_EQ(props.size(), 102);
+  EXPECT_EQ(props_.size(), 102);
 
-  props.erase(0);
+  Erase(0);
-  EXPECT_EQ(props.size(), 101);
+  EXPECT_EQ(props_.size(), 101);
-  props.erase(0);
+  Erase(0);
-  EXPECT_EQ(props.size(), 101);
+  EXPECT_EQ(props_.size(), 101);
-  props.erase(1);
+  Erase(1);
-  EXPECT_EQ(props.size(), 100);
+  EXPECT_EQ(props_.size(), 100);
 }
 
-TEST(PropertyValueStore, InsertRetrieveList) {
+TEST_F(PropertyValueStoreTest, InsertRetrieveList) {
-  PropertyValueStore<> props;
+  Set(0, std::vector<PropertyValue>{1, true, 2.5, "something",
-  props.set(0, std::vector<PropertyValue>{1, true, 2.5, "something",
+                                    PropertyValue::Null});
-                                          PropertyValue::Null});
+  auto p = At(0);
-  auto p = props.at(0);
 
   EXPECT_EQ(p.type(), PropertyValue::Type::List);
   auto l = p.Value<std::vector<PropertyValue>>();
@@ -124,12 +125,11 @@ TEST(PropertyValueStore, InsertRetrieveList) {
   EXPECT_EQ(l[4].type(), PropertyValue::Type::Null);
 }
 
-TEST(PropertyValueStore, InsertRetrieveMap) {
+TEST_F(PropertyValueStoreTest, InsertRetrieveMap) {
-  PropertyValueStore<> props;
+  Set(0, std::map<std::string, PropertyValue>{
-  props.set(0, std::map<std::string, PropertyValue>{
+             {"a", 1}, {"b", true}, {"c", "something"}});
-                   {"a", 1}, {"b", true}, {"c", "something"}});
 
-  auto p = props.at(0);
+  auto p = At(0);
   EXPECT_EQ(p.type(), PropertyValue::Type::Map);
   auto m = p.Value<std::map<std::string, PropertyValue>>();
   EXPECT_EQ(m.size(), 3);