From 2954276ca827753b668cad33a0959618f323ad9b Mon Sep 17 00:00:00 2001
From: florijan <florijan@memgraph.io>
Date: Thu, 24 Aug 2017 00:13:26 +0200
Subject: [PATCH] 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 | 5 ++-
docs/user_technical/data-types.md | 11 +++--
src/database/indexes/label_property_index.hpp | 20 +++++++--
src/query/plan/operator.cpp | 6 +++
src/query/typed_value.cpp | 21 +++++++---
src/storage/property_value.cpp | 42 ++++++++++++++++---
src/storage/property_value.hpp | 9 +++-
.../features/map_operations.feature | 13 ++++--
tests/unit/graph_db_accessor_index_api.cpp | 33 ++++++++++++++-
tests/unit/property_value_store.cpp | 27 ++++++++++--
10 files changed, 159 insertions(+), 28 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ac6cf1c84..952fe0aba 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,8 +4,9 @@
### Major Features and Improvements
-* CASE construct (without aggregations)
-* rand() function added
+* CASE construct (without aggregations).
+* `rand` function added.
+* Maps can now be stored as vertex/edge properties.
### Bug Fixes and Other Changes
diff --git a/docs/user_technical/data-types.md b/docs/user_technical/data-types.md
index 5311a24fc..ca13405ee 100644
--- a/docs/user_technical/data-types.md
+++ b/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"
diff --git a/src/database/indexes/label_property_index.hpp b/src/database/indexes/label_property_index.hpp
index a27724b4b..d75b8dda3 100644
--- a/src/database/indexes/label_property_index.hpp
+++ b/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
- * index exists.
+ * @brief - Insert value, vlist, vertex into corresponding index (key) if
+ * 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,
diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp
index 27d5839ba..dfe56c52d 100644
--- a/src/query/plan/operator.cpp
+++ b/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");
diff --git a/src/query/typed_value.cpp b/src/query/typed_value.cpp
index de8c7512b..5eee0c57b 100644
--- a/src/query/typed_value.cpp
+++ b/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(
- "Unsupported conversion from TypedValue to PropertyValue");
+ break;
}
+ 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;
}
}
diff --git a/src/storage/property_value.cpp b/src/storage/property_value.cpp
index c27247a15..ff7d5f877 100644
--- a/src/storage/property_value.cpp
+++ b/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");
}
diff --git a/src/storage/property_value.hpp b/src/storage/property_value.hpp
index 7258c6ed8..555c07e4d 100644
--- a/src/storage/property_value.hpp
+++ b/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;
};
/**
diff --git a/tests/qa/tck_engine/tests/memgraph_V1/features/map_operations.feature b/tests/qa/tck_engine/tests/memgraph_V1/features/map_operations.feature
index c6569154f..9b460e8dc 100644
--- a/tests/qa/tck_engine/tests/memgraph_V1/features/map_operations.feature
+++ b/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
diff --git a/tests/unit/graph_db_accessor_index_api.cpp b/tests/unit/graph_db_accessor_index_api.cpp
index 514669a5c..920cb0867 100644
--- a/tests/unit/graph_db_accessor_index_api.cpp
+++ b/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.");
}
diff --git a/tests/unit/property_value_store.cpp b/tests/unit/property_value_store.cpp
index 2984ecbda..dc461e7ae 100644
--- a/tests/unit/property_value_store.cpp
+++ b/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 =
- [&](const uint32_t, const PropertyValue&) { count_props += 1; };
+ auto handler = [&](const uint32_t, const PropertyValue &) {
+ 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");
+}