From eed83a210e0618cde2f0dd8211f4e11598a1451b Mon Sep 17 00:00:00 2001 From: Tonko Sabolcec Date: Mon, 2 Mar 2020 12:20:29 +0100 Subject: [PATCH] Implement unique constraint functionality in query module Reviewers: mferencevic Reviewed By: mferencevic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2681 --- src/query/interpreter.cpp | 138 +++++++++++++++++++++++--- src/storage/v2/constraints.cpp | 22 +++- src/storage/v2/constraints.hpp | 31 ++++-- src/storage/v2/storage.cpp | 4 +- src/storage/v2/storage.hpp | 20 ++-- tests/unit/interpreter.cpp | 72 ++++++++++++++ tests/unit/storage_v2_constraints.cpp | 44 +++++--- 7 files changed, 283 insertions(+), 48 deletions(-) diff --git a/src/query/interpreter.cpp b/src/query/interpreter.cpp index 39e17b8b8..f871d4e3c 100644 --- a/src/query/interpreter.cpp +++ b/src/query/interpreter.cpp @@ -20,6 +20,7 @@ #ifdef MG_SINGLE_NODE_HA #include "raft/exceptions.hpp" #endif +#include "utils/algorithm.hpp" #include "utils/exceptions.hpp" #include "utils/flag_validation.hpp" #include "utils/string.hpp" @@ -857,12 +858,22 @@ PreparedQuery PrepareInfoQuery( auto *db = interpreter_context->db; auto info = db->ListAllConstraints(); std::vector> results; - results.reserve(info.existence.size()); + results.reserve(info.existence.size() + info.unique.size()); for (const auto &item : info.existence) { results.push_back({TypedValue("exists"), TypedValue(db->LabelToName(item.first)), TypedValue(db->PropertyToName(item.second))}); } + for (const auto &item : info.unique) { + std::stringstream properties; + utils::PrintIterable(properties, item.second, ", ", + [&db](auto &stream, const auto &entry) { + stream << db->PropertyToName(entry); + }); + results.push_back({TypedValue("unique"), + TypedValue(db->LabelToName(item.first)), + TypedValue(properties.str())}); + } return std::pair{results, QueryHandlerResult::NOTHING}; }; break; @@ -935,14 +946,61 @@ PreparedQuery PrepareConstraintQuery( interpreter_context->db->PropertyToName( *violation.properties.begin()); throw QueryRuntimeException( - "Unable to create a constraint :{}({}), because an existing " - "node violates it.", + "Unable to create existence constraint :{}({}), because an " + "existing node violates it.", label_name, property_name); } }; break; case Constraint::Type::UNIQUE: - throw utils::NotYetImplemented("Unique constraints"); + std::set property_set; + for (const auto &property : properties) { + property_set.insert(property); + } + if (property_set.size() != properties.size()) { + throw SyntaxException( + "The given set of properties contains duplicates."); + } + handler = [interpreter_context, label, + property_set = std::move(property_set)] { + auto res = interpreter_context->db->CreateUniqueConstraint( + label, property_set); + if (res.HasError()) { + auto violation = res.GetError(); + auto label_name = + interpreter_context->db->LabelToName(violation.label); + std::stringstream property_names_stream; + utils::PrintIterable( + property_names_stream, violation.properties, ", ", + [&interpreter_context](auto &stream, const auto &prop) { + stream << interpreter_context->db->PropertyToName(prop); + }); + throw QueryRuntimeException( + "Unable to create unique constraint :{}({}), because an " + "existing node violates it.", + label_name, property_names_stream.str()); + } else { + switch (res.GetValue()) { + case storage::UniqueConstraints::CreationStatus:: + EMPTY_PROPERTIES: + throw SyntaxException( + "At least one property must be used for unique " + "constraints."); + break; + case storage::UniqueConstraints::CreationStatus:: + PROPERTIES_SIZE_LIMIT_EXCEEDED: + throw SyntaxException( + "Too many properties specified. Limit of {} properties " + "for unique constraints is exceeded.", + storage::kUniqueConstraintsMaxProperties); + break; + case storage::UniqueConstraints::CreationStatus::ALREADY_EXISTS: + case storage::UniqueConstraints::CreationStatus::SUCCESS: + break; + } + } + }; + break; } } break; case ConstraintQuery::ActionType::DROP: { @@ -962,7 +1020,37 @@ PreparedQuery PrepareConstraintQuery( }; break; case Constraint::Type::UNIQUE: - throw utils::NotYetImplemented("Unique constraints"); + std::set property_set; + for (const auto &property : properties) { + property_set.insert(property); + } + if (property_set.size() != properties.size()) { + throw SyntaxException( + "The given set of properties contains duplicates."); + } + handler = [interpreter_context, label, + property_set = std::move(property_set)] { + auto res = interpreter_context->db->DropUniqueConstraint( + label, property_set); + switch (res) { + case storage::UniqueConstraints::DeletionStatus::EMPTY_PROPERTIES: + throw SyntaxException( + "At least one property must be used for unique " + "constraints."); + break; + case storage::UniqueConstraints::DeletionStatus:: + PROPERTIES_SIZE_LIMIT_EXCEEDED: + throw SyntaxException( + "Too many properties specified. Limit of {} properties for " + "unique constraints is exceeded.", + storage::kUniqueConstraintsMaxProperties); + break; + case storage::UniqueConstraints::DeletionStatus::NOT_FOUND: + case storage::UniqueConstraints::DeletionStatus::SUCCESS: + break; + } + return std::vector>(); + }; } } break; } @@ -1111,17 +1199,37 @@ void Interpreter::Commit() { auto maybe_constraint_violation = db_accessor_->Commit(); if (maybe_constraint_violation.HasError()) { const auto &constraint_violation = maybe_constraint_violation.GetError(); - auto label_name = - execution_db_accessor_->LabelToName(constraint_violation.label); - CHECK(constraint_violation.properties.size() == 1U); - auto property_name = - execution_db_accessor_->PropertyToName( + switch (constraint_violation.type) { + case storage::ConstraintViolation::Type::EXISTENCE: { + auto label_name = + execution_db_accessor_->LabelToName(constraint_violation.label); + CHECK(constraint_violation.properties.size() == 1U); + auto property_name = execution_db_accessor_->PropertyToName( *constraint_violation.properties.begin()); - execution_db_accessor_ = std::nullopt; - db_accessor_ = std::nullopt; - throw QueryException( - "Unable to commit due to existence constraint violation on :{}({}).", - label_name, property_name); + execution_db_accessor_ = std::nullopt; + db_accessor_ = std::nullopt; + throw QueryException( + "Unable to commit due to existence constraint violation on :{}({})", + label_name, property_name); + break; + } + case storage::ConstraintViolation::Type::UNIQUE: { + auto label_name = + execution_db_accessor_->LabelToName(constraint_violation.label); + std::stringstream property_names_stream; + utils::PrintIterable( + property_names_stream, constraint_violation.properties, ", ", + [this](auto &stream, const auto &prop) { + stream << execution_db_accessor_->PropertyToName(prop); + }); + execution_db_accessor_ = std::nullopt; + db_accessor_ = std::nullopt; + throw QueryException( + "Unable to commit due to unique constraint violation on :{}({})", + label_name, property_names_stream.str()); + break; + } + } } execution_db_accessor_ = std::nullopt; db_accessor_ = std::nullopt; diff --git a/src/storage/v2/constraints.cpp b/src/storage/v2/constraints.cpp index cb4635c84..471653aef 100644 --- a/src/storage/v2/constraints.cpp +++ b/src/storage/v2/constraints.cpp @@ -353,9 +353,11 @@ utils::BasicResult UniqueConstraints::CreateConstraint( LabelId label, const std::set &properties, utils::SkipList::Accessor vertices) { - if (properties.empty() || - properties.size() > kUniqueConstraintsMaxProperties) { - return CreationStatus::INVALID_PROPERTIES_SIZE; + if (properties.empty()) { + return CreationStatus::EMPTY_PROPERTIES; + } + if (properties.size() > kUniqueConstraintsMaxProperties) { + return CreationStatus::PROPERTIES_SIZE_LIMIT_EXCEEDED; } auto [constraint, emplaced] = constraints_.emplace( @@ -404,6 +406,20 @@ UniqueConstraints::CreateConstraint( return CreationStatus::SUCCESS; } +UniqueConstraints::DeletionStatus UniqueConstraints::DropConstraint( + LabelId label, const std::set &properties) { + if (properties.empty()) { + return UniqueConstraints::DeletionStatus::EMPTY_PROPERTIES; + } + if (properties.size() > kUniqueConstraintsMaxProperties) { + return UniqueConstraints::DeletionStatus::PROPERTIES_SIZE_LIMIT_EXCEEDED; + } + if (constraints_.erase({label, properties}) > 0) { + return UniqueConstraints::DeletionStatus::SUCCESS; + } + return UniqueConstraints::DeletionStatus::NOT_FOUND; +} + std::optional UniqueConstraints::Validate( const Vertex &vertex, const Transaction &tx, uint64_t commit_timestamp) const { diff --git a/src/storage/v2/constraints.hpp b/src/storage/v2/constraints.hpp index 771c8ffc6..241bb0bf0 100644 --- a/src/storage/v2/constraints.hpp +++ b/src/storage/v2/constraints.hpp @@ -71,7 +71,16 @@ class UniqueConstraints { enum class CreationStatus { SUCCESS, ALREADY_EXISTS, - INVALID_PROPERTIES_SIZE, + EMPTY_PROPERTIES, + PROPERTIES_SIZE_LIMIT_EXCEEDED, + }; + + /// Status for deletion of unique constraints. + enum class DeletionStatus { + SUCCESS, + NOT_FOUND, + EMPTY_PROPERTIES, + PROPERTIES_SIZE_LIMIT_EXCEEDED, }; /// Indexes the given vertex for relevant labels and properties. @@ -83,18 +92,24 @@ class UniqueConstraints { /// Creates unique constraint on the given `label` and a list of `properties`. /// Returns constraint violation if there are multiple vertices with the same /// label and property values. Returns `CreationStatus::ALREADY_EXISTS` if - /// constraint already existed, `CreationStatus::INVALID_PROPERTY_SIZE` if - /// the given list of properties is empty or the list of properties exceeds - /// the maximum allowed number of properties, and `CreationStatus::SUCCESS` on - /// success. + /// constraint already existed, `CreationStatus::EMPTY_PROPERTIES` if the + /// given list of properties is empty, + /// `CreationStatus::PROPERTIES_SIZE_LIMIT_EXCEEDED` if the list of properties + /// exceeds the maximum allowed number of properties, and + /// `CreationStatus::SUCCESS` on success. /// @throw std::bad_alloc utils::BasicResult CreateConstraint( LabelId label, const std::set &properties, utils::SkipList::Accessor vertices); - bool DropConstraint(LabelId label, const std::set &properties) { - return constraints_.erase({label, properties}) > 0; - } + /// Deletes the specified constraint. Returns `DeletionStatus::NOT_FOUND` if + /// there is not such constraint in the storage, + /// `DeletionStatus::EMPTY_PROPERTIES` if the given set of `properties` is + /// empty, `DeletionStatus::PROPERTIES_SIZE_LIMIT_EXCEEDED` if the given set + /// of `properties` exceeds the maximum allowed number of properties, and + /// `DeletionStatus::SUCCESS` on success. + DeletionStatus DropConstraint(LabelId label, + const std::set &properties); bool ConstraintExists(LabelId label, const std::set &properties) { return constraints_.find({label, properties}) != constraints_.end(); diff --git a/src/storage/v2/storage.cpp b/src/storage/v2/storage.cpp index e6ec3ac20..c3697571b 100644 --- a/src/storage/v2/storage.cpp +++ b/src/storage/v2/storage.cpp @@ -1066,8 +1066,8 @@ Storage::CreateUniqueConstraint(LabelId label, vertices_.access()); } -bool Storage::DropUniqueConstraint(LabelId label, - const std::set &properties) { +UniqueConstraints::DeletionStatus Storage::DropUniqueConstraint( + LabelId label, const std::set &properties) { std::unique_lock storage_guard(main_lock_); // TODO(tsabolcec): Append action to the WAL. return constraints_.unique_constraints.DropConstraint(label, properties); diff --git a/src/storage/v2/storage.hpp b/src/storage/v2/storage.hpp index 4e82da7a7..2b9e73161 100644 --- a/src/storage/v2/storage.hpp +++ b/src/storage/v2/storage.hpp @@ -355,18 +355,24 @@ class Storage final { /// constraint, it returns `ConstraintViolation`. Otherwise returns a /// `UniqueConstraints::CreationStatus` enum with the following possibilities: /// * `SUCCESS` if the constraint was successfully created, - /// * `ALREADY_EXISTS` if the constraint already existed, or - /// * `INVALID_PROPERTIES_SIZE` if the property set is empty or exceeds - /// the limit of maximum number of properties. + /// * `ALREADY_EXISTS` if the constraint already existed, + /// * `EMPTY_PROPERTIES` if the property set is empty, or + // * `PROPERTIES_SIZE_LIMIT_EXCEEDED` if the property set exceeds the + // limit of maximum number of properties. /// /// @throw std::bad_alloc utils::BasicResult CreateUniqueConstraint(LabelId label, const std::set &properties); - /// Removes a unique constraint. Returns true if the constraint was removed, - /// and false if it doesn't exist. - bool DropUniqueConstraint(LabelId label, - const std::set &properties); + /// Removes a unique constraint. Returns `UniqueConstraints::DeletionStatus` + /// enum with the following possibilities: + /// * `SUCCESS` if constraint was successfully removed, + /// * `NOT_FOUND` if the specified constraint was not found, + /// * `EMPTY_PROPERTIES` if the property set is empty, or + /// * `PROPERTIES_SIZE_LIMIT_EXCEEDED` if the property set exceeds the + // limit of maximum number of properties. + UniqueConstraints::DeletionStatus DropUniqueConstraint( + LabelId label, const std::set &properties); ConstraintsInfo ListAllConstraints() const; diff --git a/tests/unit/interpreter.cpp b/tests/unit/interpreter.cpp index 77926e7fe..c7e87b96b 100644 --- a/tests/unit/interpreter.cpp +++ b/tests/unit/interpreter.cpp @@ -337,6 +337,78 @@ TEST_F(InterpreterTest, ExistenceConstraintTest) { Interpret("CREATE (:A{a:2})"); Interpret("MATCH (n:A{a:2}) DETACH DELETE n"); Interpret("CREATE (n:A{a:2})"); + ASSERT_THROW(Interpret("CREATE CONSTRAINT ON (n:A) ASSERT EXISTS (n.b);"), + query::QueryRuntimeException); +} + +TEST_F(InterpreterTest, UniqueConstraintTest) { + // Empty property list should result with syntax exception. + ASSERT_THROW(Interpret("CREATE CONSTRAINT ON (n:A) ASSERT IS UNIQUE;"), + query::SyntaxException); + ASSERT_THROW(Interpret("DROP CONSTRAINT ON (n:A) ASSERT IS UNIQUE;"), + query::SyntaxException); + + // Too large list of properties should also result with syntax exception. + { + std::stringstream stream; + stream << " ON (n:A) ASSERT "; + for (size_t i = 0; i < 33; ++i) { + if (i > 0) stream << ", "; + stream << "n.prop" << i; + } + stream << " IS UNIQUE;"; + std::string create_query = "CREATE CONSTRAINT" + stream.str(); + std::string drop_query = "DROP CONSTRAINT" + stream.str(); + ASSERT_THROW(Interpret(create_query), query::SyntaxException); + ASSERT_THROW(Interpret(drop_query), query::SyntaxException); + } + + // Providing property list with duplicates results with syntax exception. + ASSERT_THROW( + Interpret("CREATE CONSTRAINT ON (n:A) ASSERT n.a, n.b, n.a IS UNIQUE;"), + query::SyntaxException); + ASSERT_THROW( + Interpret("DROP CONSTRAINT ON (n:A) ASSERT n.a, n.b, n.a IS UNIQUE;"), + query::SyntaxException); + + // Commit of vertex should fail if a constraint is violated. + Interpret("CREATE CONSTRAINT ON (n:A) ASSERT n.a, n.b IS UNIQUE;"); + Interpret("CREATE (:A{a:1, b:2})"); + Interpret("CREATE (:A{a:1, b:3})"); + ASSERT_THROW(Interpret("CREATE (:A{a:1, b:2})"), query::QueryException); + + // Attempt to create a constraint should fail if it's violated. + Interpret("CREATE (:A{a:1, c:2})"); + Interpret("CREATE (:A{a:1, c:2})"); + ASSERT_THROW( + Interpret("CREATE CONSTRAINT ON (n:A) ASSERT n.a, n.c IS UNIQUE;"), + query::QueryRuntimeException); + + Interpret("MATCH (n:A{a:2, b:2}) SET n.a=1"); + Interpret("CREATE (:A{a:2})"); + Interpret("MATCH (n:A{a:2}) DETACH DELETE n"); + Interpret("CREATE (n:A{a:2})"); + + // Show constraint info. + { + auto stream = Interpret("SHOW CONSTRAINT INFO"); + ASSERT_EQ(stream.GetHeader().size(), 3U); + const auto &header = stream.GetHeader(); + ASSERT_EQ(header[0], "constraint type"); + ASSERT_EQ(header[1], "label"); + ASSERT_EQ(header[2], "properties"); + ASSERT_EQ(stream.GetResults().size(), 1U); + const auto &result = stream.GetResults().front(); + ASSERT_EQ(result.size(), 3U); + ASSERT_EQ(result[0].ValueString(), "unique"); + ASSERT_EQ(result[1].ValueString(), "A"); + ASSERT_EQ(result[2].ValueString(), "a, b"); + } + + // Drop constraint. + Interpret("DROP CONSTRAINT ON (n:A) ASSERT n.a, n.b IS UNIQUE;"); + // Removing the same constraint twice should not throw any exception. + Interpret("DROP CONSTRAINT ON (n:A) ASSERT n.a, n.b IS UNIQUE;"); } TEST_F(InterpreterTest, ExplainQuery) { diff --git a/tests/unit/storage_v2_constraints.cpp b/tests/unit/storage_v2_constraints.cpp index 9cb9741ad..470a53c7d 100644 --- a/tests/unit/storage_v2_constraints.cpp +++ b/tests/unit/storage_v2_constraints.cpp @@ -182,7 +182,7 @@ TEST_F(ConstraintsTest, ExistenceConstraintsViolationOnCommit) { } // NOLINTNEXTLINE(hicpp-special-member-functions) -TEST_F(ConstraintsTest, UniqueConstraintsCreateAndDrop) { +TEST_F(ConstraintsTest, UniqueConstraintsCreateAndDropAndList) { EXPECT_EQ(storage.ListAllConstraints().unique.size(), 0); { auto res = storage.CreateUniqueConstraint(label1, {prop1}); @@ -211,13 +211,17 @@ TEST_F(ConstraintsTest, UniqueConstraintsCreateAndDrop) { UnorderedElementsAre( std::make_pair(label1, std::set{prop1}), std::make_pair(label2, std::set{prop1}))); - EXPECT_TRUE(storage.DropUniqueConstraint(label1, {prop1})); - EXPECT_FALSE(storage.DropUniqueConstraint(label1, {prop1})); + EXPECT_EQ(storage.DropUniqueConstraint(label1, {prop1}), + UniqueConstraints::DeletionStatus::SUCCESS); + EXPECT_EQ(storage.DropUniqueConstraint(label1, {prop1}), + UniqueConstraints::DeletionStatus::NOT_FOUND); EXPECT_THAT(storage.ListAllConstraints().unique, UnorderedElementsAre( std::make_pair(label2, std::set{prop1}))); - EXPECT_TRUE(storage.DropUniqueConstraint(label2, {prop1})); - EXPECT_FALSE(storage.DropUniqueConstraint(label2, {prop2})); + EXPECT_EQ(storage.DropUniqueConstraint(label2, {prop1}), + UniqueConstraints::DeletionStatus::SUCCESS); + EXPECT_EQ(storage.DropUniqueConstraint(label2, {prop2}), + UniqueConstraints::DeletionStatus::NOT_FOUND); EXPECT_EQ(storage.ListAllConstraints().unique.size(), 0); { auto res = storage.CreateUniqueConstraint(label2, {prop1}); @@ -618,7 +622,6 @@ TEST_F(ConstraintsTest, UniqueConstraintsLabelAlteration) { ASSERT_NO_ERROR(vertex1->RemoveLabel(label1)); ASSERT_NO_ERROR(vertex1->AddLabel(label1)); - // Commit the first transaction. ASSERT_NO_ERROR(acc1.Commit()); } @@ -682,9 +685,13 @@ TEST_F(ConstraintsTest, UniqueConstraintsPropertySetSize) { auto res = storage.CreateUniqueConstraint(label1, {}); ASSERT_TRUE(res.HasValue()); ASSERT_EQ(res.GetValue(), - UniqueConstraints::CreationStatus::INVALID_PROPERTIES_SIZE); + UniqueConstraints::CreationStatus::EMPTY_PROPERTIES); } + // Removing a constraint with empty property set should also fail. + ASSERT_EQ(storage.DropUniqueConstraint(label1, {}), + UniqueConstraints::DeletionStatus::EMPTY_PROPERTIES); + // Create a set of 33 properties. std::set properties; for (int i = 1; i <= 33; ++i) { @@ -696,10 +703,15 @@ TEST_F(ConstraintsTest, UniqueConstraintsPropertySetSize) { // properties, which is 32. auto res = storage.CreateUniqueConstraint(label1, properties); ASSERT_TRUE(res.HasValue()); - ASSERT_EQ(res.GetValue(), - UniqueConstraints::CreationStatus::INVALID_PROPERTIES_SIZE); + ASSERT_EQ( + res.GetValue(), + UniqueConstraints::CreationStatus::PROPERTIES_SIZE_LIMIT_EXCEEDED); } + // An attempt to delete constraint with too large property set should fail. + ASSERT_EQ(storage.DropUniqueConstraint(label1, properties), + UniqueConstraints::DeletionStatus::PROPERTIES_SIZE_LIMIT_EXCEEDED); + // Remove one property from the set. properties.erase(properties.begin()); @@ -712,6 +724,11 @@ TEST_F(ConstraintsTest, UniqueConstraintsPropertySetSize) { EXPECT_THAT(storage.ListAllConstraints().unique, UnorderedElementsAre(std::make_pair(label1, properties))); + + // Removing a constraint with 32 properties should succeed. + ASSERT_EQ(storage.DropUniqueConstraint(label1, properties), + UniqueConstraints::DeletionStatus::SUCCESS); + ASSERT_TRUE(storage.ListAllConstraints().unique.empty()); } // NOLINTNEXTLINE(hicpp-special-member-functions) @@ -936,7 +953,8 @@ TEST_F(ConstraintsTest, UniqueConstraintsInsertDropInsert) { ASSERT_NO_ERROR(acc.Commit()); } - ASSERT_TRUE(storage.DropUniqueConstraint(label1, {prop2, prop1})); + ASSERT_EQ(storage.DropUniqueConstraint(label1, {prop2, prop1}), + UniqueConstraints::DeletionStatus::SUCCESS); { auto acc = storage.Access(); @@ -949,10 +967,10 @@ TEST_F(ConstraintsTest, UniqueConstraintsInsertDropInsert) { } TEST_F(ConstraintsTest, UniqueConstraintsComparePropertyValues) { - // Purpose of this test is to make sure that extracted property values - // are correctly compared. + // Purpose of this test is to make sure that extracted property values + // are correctly compared. - { + { auto res = storage.CreateUniqueConstraint(label1, {prop1, prop2}); ASSERT_TRUE(res.HasValue()); ASSERT_EQ(res.GetValue(), UniqueConstraints::CreationStatus::SUCCESS);