Invert the result of SetProperty and add documentation

Reviewers: mtomic, mferencevic

Reviewed By: mferencevic

Subscribers: pullbot

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

src/storage/v2/edge_accessor.cpp

@@ -26,6 +26,12 @@ Result<bool> EdgeAccessor::SetProperty(PropertyId property,
 
   auto it = edge_->properties.find(property);
   bool existed = it != edge_->properties.end();
+  // We could skip setting the value if the previous one is the same to the new
+  // one. This would save some memory as a delta would not be created as well as
+  // avoid copying the value. The reason we are not doing that is because the
+  // current code always follows the logical pattern of "create a delta" and
+  // "modify in-place". Additionally, the created delta will make other
+  // transactions get a SERIALIZATION_ERROR.
   if (it != edge_->properties.end()) {
     CreateAndLinkDelta(transaction_, edge_, Delta::SetPropertyTag(), property,
                        it->second);
@@ -44,7 +50,7 @@ Result<bool> EdgeAccessor::SetProperty(PropertyId property,
     }
   }
 
-  return existed;
+  return !existed;
 }
 
 Result<PropertyValue> EdgeAccessor::GetProperty(PropertyId property,

src/storage/v2/edge_accessor.hpp

@@ -34,6 +34,9 @@ class EdgeAccessor final {
 
   EdgeTypeId EdgeType() const { return edge_type_; }
 
+  /// Set a property value and return `true` if insertion took place.
+  /// `false` is returned if assignment took place.
+  /// @throw std::bad_alloc
   Result<bool> SetProperty(PropertyId property, const PropertyValue &value);
 
   Result<PropertyValue> GetProperty(PropertyId property, View view) const;

src/storage/v2/vertex_accessor.cpp

@@ -190,6 +190,12 @@ Result<bool> VertexAccessor::SetProperty(PropertyId property,
 
   auto it = vertex_->properties.find(property);
   bool existed = it != vertex_->properties.end();
+  // We could skip setting the value if the previous one is the same to the new
+  // one. This would save some memory as a delta would not be created as well as
+  // avoid copying the value. The reason we are not doing that is because the
+  // current code always follows the logical pattern of "create a delta" and
+  // "modify in-place". Additionally, the created delta will make other
+  // transactions get a SERIALIZATION_ERROR.
   if (it != vertex_->properties.end()) {
     CreateAndLinkDelta(transaction_, vertex_, Delta::SetPropertyTag(), property,
                        it->second);
@@ -210,7 +216,7 @@ Result<bool> VertexAccessor::SetProperty(PropertyId property,
 
   UpdateOnSetProperty(indices_, property, value, vertex_, *transaction_);
 
-  return existed;
+  return !existed;
 }
 
 Result<PropertyValue> VertexAccessor::GetProperty(PropertyId property,

src/storage/v2/vertex_accessor.hpp

@@ -34,6 +34,9 @@ class VertexAccessor final {
 
   Result<std::vector<LabelId>> Labels(View view) const;
 
+  /// Set a property value and return `true` if insertion took place.
+  /// `false` is returned if assignment took place.
+  /// @throw std::bad_alloc
   Result<bool> SetProperty(PropertyId property, const PropertyValue &value);
 
   Result<PropertyValue> GetProperty(PropertyId property, View view) const;

tests/unit/storage_v2.cpp

@@ -743,7 +743,7 @@ TEST(StorageV2, VertexDeleteProperty) {
     ASSERT_EQ(vertex->Properties(storage::View::NEW)->size(), 0);
 
     // Set property 5 to "nandare"
-    ASSERT_FALSE(
+    ASSERT_TRUE(
         vertex->SetProperty(property, storage::PropertyValue("nandare"))
             .GetValue());
 
@@ -796,7 +796,7 @@ TEST(StorageV2, VertexDeleteProperty) {
     ASSERT_EQ(vertex->Properties(storage::View::NEW)->size(), 0);
 
     // Set property 5 to "nandare"
-    ASSERT_FALSE(
+    ASSERT_TRUE(
         vertex->SetProperty(property, storage::PropertyValue("nandare"))
             .GetValue());
 
@@ -1364,7 +1364,7 @@ TEST(StorageV2, VertexPropertyCommit) {
       auto res =
           vertex.SetProperty(property, storage::PropertyValue("temporary"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_EQ(vertex.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -1379,7 +1379,7 @@ TEST(StorageV2, VertexPropertyCommit) {
       auto res =
           vertex.SetProperty(property, storage::PropertyValue("nandare"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(vertex.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -1434,7 +1434,7 @@ TEST(StorageV2, VertexPropertyCommit) {
     {
       auto res = vertex->SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(vertex->GetProperty(property, storage::View::OLD)->ValueString(),
@@ -1451,7 +1451,7 @@ TEST(StorageV2, VertexPropertyCommit) {
     {
       auto res = vertex->SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     acc.Commit();
@@ -1508,7 +1508,7 @@ TEST(StorageV2, VertexPropertyAbort) {
       auto res =
           vertex->SetProperty(property, storage::PropertyValue("temporary"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_EQ(vertex->GetProperty(property, storage::View::NEW)->ValueString(),
@@ -1523,7 +1523,7 @@ TEST(StorageV2, VertexPropertyAbort) {
       auto res =
           vertex->SetProperty(property, storage::PropertyValue("nandare"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(vertex->GetProperty(property, storage::View::NEW)->ValueString(),
@@ -1575,7 +1575,7 @@ TEST(StorageV2, VertexPropertyAbort) {
       auto res =
           vertex->SetProperty(property, storage::PropertyValue("temporary"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_EQ(vertex->GetProperty(property, storage::View::NEW)->ValueString(),
@@ -1590,7 +1590,7 @@ TEST(StorageV2, VertexPropertyAbort) {
       auto res =
           vertex->SetProperty(property, storage::PropertyValue("nandare"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(vertex->GetProperty(property, storage::View::NEW)->ValueString(),
@@ -1665,7 +1665,7 @@ TEST(StorageV2, VertexPropertyAbort) {
     {
       auto res = vertex->SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(vertex->GetProperty(property, storage::View::OLD)->ValueString(),
@@ -1743,7 +1743,7 @@ TEST(StorageV2, VertexPropertyAbort) {
     {
       auto res = vertex->SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(vertex->GetProperty(property, storage::View::OLD)->ValueString(),
@@ -1817,7 +1817,7 @@ TEST(StorageV2, VertexPropertySerializationError) {
     {
       auto res = vertex->SetProperty(property1, storage::PropertyValue(123));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_TRUE(vertex->GetProperty(property1, storage::View::OLD)->IsNull());
@@ -1941,7 +1941,7 @@ TEST(StorageV2, VertexLabelPropertyMixed) {
   ASSERT_EQ(vertex.Properties(storage::View::NEW)->size(), 0);
 
   // Set property 5 to "nandare"
-  ASSERT_FALSE(vertex.SetProperty(property, storage::PropertyValue("nandare"))
+  ASSERT_TRUE(vertex.SetProperty(property, storage::PropertyValue("nandare"))
                    .GetValue());
 
   // Check whether label 5 and property 5 exist
@@ -1999,7 +1999,7 @@ TEST(StorageV2, VertexLabelPropertyMixed) {
   }
 
   // Set property 5 to "haihai"
-  ASSERT_TRUE(vertex.SetProperty(property, storage::PropertyValue("haihai"))
+  ASSERT_FALSE(vertex.SetProperty(property, storage::PropertyValue("haihai"))
                   .GetValue());
 
   // Check whether label 5 and property 5 exist
@@ -2112,7 +2112,7 @@ TEST(StorageV2, VertexLabelPropertyMixed) {
   }
 
   // Set property 5 to null
-  ASSERT_TRUE(
+  ASSERT_FALSE(
       vertex.SetProperty(property, storage::PropertyValue()).GetValue());
 
   // Check whether label 5 and property 5 exist

tests/unit/storage_v2_edge.cpp

@@ -4143,7 +4143,7 @@ TEST(StorageV2, EdgePropertyCommit) {
       auto res =
           edge.SetProperty(property, storage::PropertyValue("temporary"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -4157,7 +4157,7 @@ TEST(StorageV2, EdgePropertyCommit) {
     {
       auto res = edge.SetProperty(property, storage::PropertyValue("nandare"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -4212,7 +4212,7 @@ TEST(StorageV2, EdgePropertyCommit) {
     {
       auto res = edge.SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::OLD)->ValueString(),
@@ -4229,7 +4229,7 @@ TEST(StorageV2, EdgePropertyCommit) {
     {
       auto res = edge.SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     acc.Commit();
@@ -4291,7 +4291,7 @@ TEST(StorageV2, EdgePropertyAbort) {
       auto res =
           edge.SetProperty(property, storage::PropertyValue("temporary"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -4305,7 +4305,7 @@ TEST(StorageV2, EdgePropertyAbort) {
     {
       auto res = edge.SetProperty(property, storage::PropertyValue("nandare"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -4357,7 +4357,7 @@ TEST(StorageV2, EdgePropertyAbort) {
       auto res =
           edge.SetProperty(property, storage::PropertyValue("temporary"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -4371,7 +4371,7 @@ TEST(StorageV2, EdgePropertyAbort) {
     {
       auto res = edge.SetProperty(property, storage::PropertyValue("nandare"));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::NEW)->ValueString(),
@@ -4446,7 +4446,7 @@ TEST(StorageV2, EdgePropertyAbort) {
     {
       auto res = edge.SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::OLD)->ValueString(),
@@ -4524,7 +4524,7 @@ TEST(StorageV2, EdgePropertyAbort) {
     {
       auto res = edge.SetProperty(property, storage::PropertyValue());
       ASSERT_TRUE(res.HasValue());
-      ASSERT_TRUE(res.GetValue());
+      ASSERT_FALSE(res.GetValue());
     }
 
     ASSERT_EQ(edge.GetProperty(property, storage::View::OLD)->ValueString(),
@@ -4603,7 +4603,7 @@ TEST(StorageV2, EdgePropertySerializationError) {
     {
       auto res = edge.SetProperty(property1, storage::PropertyValue(123));
       ASSERT_TRUE(res.HasValue());
-      ASSERT_FALSE(res.GetValue());
+      ASSERT_TRUE(res.GetValue());
     }
 
     ASSERT_TRUE(edge.GetProperty(property1, storage::View::OLD)->IsNull());