From ce9e2614fa021ba3b4a356852c44973521a945f0 Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Tue, 13 Aug 2019 13:26:38 +0200 Subject: [PATCH] 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 | 8 +++++++- src/storage/v2/edge_accessor.hpp | 3 +++ src/storage/v2/vertex_accessor.cpp | 8 +++++++- src/storage/v2/vertex_accessor.hpp | 3 +++ tests/unit/storage_v2.cpp | 32 +++++++++++++++--------------- tests/unit/storage_v2_edge.cpp | 22 ++++++++++---------- 6 files changed, 47 insertions(+), 29 deletions(-) diff --git a/src/storage/v2/edge_accessor.cpp b/src/storage/v2/edge_accessor.cpp index 5a8b7df00..04f03a5ea 100644 --- a/src/storage/v2/edge_accessor.cpp +++ b/src/storage/v2/edge_accessor.cpp @@ -26,6 +26,12 @@ Result 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 EdgeAccessor::SetProperty(PropertyId property, } } - return existed; + return !existed; } Result EdgeAccessor::GetProperty(PropertyId property, diff --git a/src/storage/v2/edge_accessor.hpp b/src/storage/v2/edge_accessor.hpp index ec0abe392..9fa0be443 100644 --- a/src/storage/v2/edge_accessor.hpp +++ b/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 SetProperty(PropertyId property, const PropertyValue &value); Result GetProperty(PropertyId property, View view) const; diff --git a/src/storage/v2/vertex_accessor.cpp b/src/storage/v2/vertex_accessor.cpp index 7cf4d8e7a..a761df9d7 100644 --- a/src/storage/v2/vertex_accessor.cpp +++ b/src/storage/v2/vertex_accessor.cpp @@ -190,6 +190,12 @@ Result 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 VertexAccessor::SetProperty(PropertyId property, UpdateOnSetProperty(indices_, property, value, vertex_, *transaction_); - return existed; + return !existed; } Result VertexAccessor::GetProperty(PropertyId property, diff --git a/src/storage/v2/vertex_accessor.hpp b/src/storage/v2/vertex_accessor.hpp index 2acc2ad32..bd86ce197 100644 --- a/src/storage/v2/vertex_accessor.hpp +++ b/src/storage/v2/vertex_accessor.hpp @@ -34,6 +34,9 @@ class VertexAccessor final { Result> 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 SetProperty(PropertyId property, const PropertyValue &value); Result GetProperty(PropertyId property, View view) const; diff --git a/tests/unit/storage_v2.cpp b/tests/unit/storage_v2.cpp index c0bc69c1e..be474f03b 100644 --- a/tests/unit/storage_v2.cpp +++ b/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 diff --git a/tests/unit/storage_v2_edge.cpp b/tests/unit/storage_v2_edge.cpp index 070bb6cd9..a77c551e8 100644 --- a/tests/unit/storage_v2_edge.cpp +++ b/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());