Finalize storage v2 index API

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

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

src/query/db_accessor.hpp

@@ -604,20 +604,13 @@ class DbAccessor final {
 
   bool MustAbort() const { return false; }
 
-  // TODO: Index manipulation should not go through Accessor
+  bool LabelIndexExists(storage::Label label) const {
-  bool CreateIndex(storage::Label label, storage::Property prop) {
+    return accessor_->LabelIndexExists(label);
-    throw utils::NotYetImplemented("CreateIndex");
   }
 
-  bool DropIndex(storage::Label label, storage::Property prop) {
-    throw utils::NotYetImplemented("DropIndex");
-  }
-
-  // TODO: Querying information should probably be in some kind of StorageInfo
-  // class instead of here in Accessor.
   bool LabelPropertyIndexExists(storage::Label label,
                                 storage::Property prop) const {
-    throw utils::NotYetImplemented("LabelPropertyIndexExists");
+    return accessor_->LabelPropertyIndexExists(label, prop);
   }
 
   int64_t VerticesCount() const { return accessor_->ApproximateVertexCount(); }
@@ -643,15 +636,12 @@ class DbAccessor final {
     return accessor_->ApproximateVertexCount(label, property, lower, upper);
   }
 
-  // TODO: Constraints manipulation should not go through Accessor
+  storage::IndicesInfo ListAllIndices() const {
-  utils::BasicResult<storage::ExistenceConstraintViolation, bool>
+    return accessor_->ListAllIndices();
-  CreateExistenceConstraint(storage::Label label, storage::Property property) {
-    throw utils::NotYetImplemented("CreateExistenceConstraint");
   }
 
-  bool DropExistenceConstraint(storage::Label label,
+  storage::ConstraintsInfo ListAllConstraints() const {
-                               storage::Property property) {
+    return accessor_->ListAllConstraints();
-    throw utils::NotYetImplemented("DropExistenceConstraint");
   }
 };
 #else
@@ -810,6 +800,11 @@ class DbAccessor final {
     }
   }
 
+  bool LabelIndexExists(storage::Label label) const {
+    // Label indices exist for all labels in the v1 storage.
+    return true;
+  }
+
   bool LabelPropertyIndexExists(storage::Label label,
                                 storage::Property prop) const {
     return dba_->LabelPropertyIndexExists(label, prop);

src/storage/v2/storage.cpp

@@ -966,6 +966,12 @@ bool Storage::DropIndex(LabelId label, PropertyId property) {
   return true;
 }
 
+IndicesInfo Storage::ListAllIndices() const {
+  std::shared_lock<utils::RWLock> storage_guard_(main_lock_);
+  return {indices_.label_index.ListIndices(),
+          indices_.label_property_index.ListIndices()};
+}
+
 utils::BasicResult<ExistenceConstraintViolation, bool>
 Storage::CreateExistenceConstraint(LabelId label, PropertyId property) {
   std::unique_lock<utils::RWLock> storage_guard(main_lock_);
@@ -990,6 +996,11 @@ bool Storage::DropExistenceConstraint(LabelId label, PropertyId property) {
   return true;
 }
 
+ConstraintsInfo Storage::ListAllConstraints() const {
+  std::shared_lock<utils::RWLock> storage_guard_(main_lock_);
+  return {ListExistenceConstraints(constraints_)};
+}
+
 VerticesIterable Storage::Accessor::Vertices(LabelId label, View view) {
   return VerticesIterable(
       storage_->indices_.label_index.Vertices(label, view, &transaction_));

src/storage/v2/storage.hpp

@@ -264,6 +264,24 @@ class Storage final {
     /// @throw std::bad_alloc if unable to insert a new mapping
     EdgeTypeId NameToEdgeType(const std::string &name);
 
+    bool LabelIndexExists(LabelId label) const {
+      return storage_->indices_.label_index.IndexExists(label);
+    }
+
+    bool LabelPropertyIndexExists(LabelId label, PropertyId property) const {
+      return storage_->indices_.label_property_index.IndexExists(label,
+                                                                 property);
+    }
+
+    IndicesInfo ListAllIndices() const {
+      return {storage_->indices_.label_index.ListIndices(),
+              storage_->indices_.label_property_index.ListIndices()};
+    }
+
+    ConstraintsInfo ListAllConstraints() const {
+      return {ListExistenceConstraints(storage_->constraints_)};
+    }
+
     void AdvanceCommand();
 
     /// Commit returns `ExistenceConstraintViolation` if the changes made by
@@ -308,18 +326,7 @@ class Storage final {
 
   bool DropIndex(LabelId label, PropertyId property);
 
-  bool LabelIndexExists(LabelId label) const {
+  IndicesInfo ListAllIndices() const;
-    return indices_.label_index.IndexExists(label);
-  }
-
-  bool LabelPropertyIndexExists(LabelId label, PropertyId property) const {
-    return indices_.label_property_index.IndexExists(label, property);
-  }
-
-  IndicesInfo ListAllIndices() const {
-    return {indices_.label_index.ListIndices(),
-            indices_.label_property_index.ListIndices()};
-  }
 
   /// Creates a unique constraint`. Returns true if the constraint was
   /// successfuly added, false if it already exists and an
@@ -335,9 +342,7 @@ class Storage final {
   /// and false if it doesn't exist.
   bool DropExistenceConstraint(LabelId label, PropertyId property);
 
-  ConstraintsInfo ListAllConstraints() const {
+  ConstraintsInfo ListAllConstraints() const;
-    return {ListExistenceConstraints(constraints_)};
-  }
 
  private:
   Transaction CreateTransaction();
@@ -352,7 +357,7 @@ class Storage final {
   // creation of new accessors by taking a unique lock. This is used when doing
   // operations on storage that affect the global state, for example index
   // creation.
-  utils::RWLock main_lock_{utils::RWLock::Priority::WRITE};
+  mutable utils::RWLock main_lock_{utils::RWLock::Priority::WRITE};
 
   // Main object storage
   utils::SkipList<storage::Vertex> vertices_;

tests/unit/storage_v2_indices.cpp

@@ -50,7 +50,10 @@ class IndexTest : public testing::Test {
 
 // NOLINTNEXTLINE(hicpp-special-member-functions)
 TEST_F(IndexTest, LabelIndexCreate) {
-  EXPECT_FALSE(storage.LabelIndexExists(label1));
+  {
+    auto acc = storage.Access();
+    EXPECT_FALSE(acc.LabelIndexExists(label1));
+  }
   EXPECT_EQ(storage.ListAllIndices().label.size(), 0);
 
   {
@@ -136,7 +139,10 @@ TEST_F(IndexTest, LabelIndexCreate) {
 
 // NOLINTNEXTLINE(hicpp-special-member-functions)
 TEST_F(IndexTest, LabelIndexDrop) {
-  EXPECT_FALSE(storage.LabelIndexExists(label1));
+  {
+    auto acc = storage.Access();
+    EXPECT_FALSE(acc.LabelIndexExists(label1));
+  }
   EXPECT_EQ(storage.ListAllIndices().label.size(), 0);
 
   {
@@ -159,11 +165,17 @@ TEST_F(IndexTest, LabelIndexDrop) {
   }
 
   EXPECT_TRUE(storage.DropIndex(label1));
-  EXPECT_FALSE(storage.LabelIndexExists(label1));
+  {
+    auto acc = storage.Access();
+    EXPECT_FALSE(acc.LabelIndexExists(label1));
+  }
   EXPECT_EQ(storage.ListAllIndices().label.size(), 0);
 
   EXPECT_FALSE(storage.DropIndex(label1));
-  EXPECT_FALSE(storage.LabelIndexExists(label1));
+  {
+    auto acc = storage.Access();
+    EXPECT_FALSE(acc.LabelIndexExists(label1));
+  }
   EXPECT_EQ(storage.ListAllIndices().label.size(), 0);
 
   {
@@ -176,7 +188,10 @@ TEST_F(IndexTest, LabelIndexDrop) {
   }
 
   EXPECT_TRUE(storage.CreateIndex(label1));
-  EXPECT_TRUE(storage.LabelIndexExists(label1));
+  {
+    auto acc = storage.Access();
+    EXPECT_TRUE(acc.LabelIndexExists(label1));
+  }
   EXPECT_THAT(storage.ListAllIndices().label, UnorderedElementsAre(label1));
 
   {
@@ -375,28 +390,43 @@ TEST_F(IndexTest, LabelIndexCountEstimate) {
 TEST_F(IndexTest, LabelPropertyIndexCreateAndDrop) {
   EXPECT_EQ(storage.ListAllIndices().label_property.size(), 0);
   EXPECT_TRUE(storage.CreateIndex(label1, prop_id));
-  EXPECT_TRUE(storage.LabelPropertyIndexExists(label1, prop_id));
+  {
+    auto acc = storage.Access();
+    EXPECT_TRUE(acc.LabelPropertyIndexExists(label1, prop_id));
+  }
   EXPECT_THAT(storage.ListAllIndices().label_property,
               UnorderedElementsAre(std::make_pair(label1, prop_id)));
-  EXPECT_FALSE(storage.LabelPropertyIndexExists(label2, prop_id));
+  {
+    auto acc = storage.Access();
+    EXPECT_FALSE(acc.LabelPropertyIndexExists(label2, prop_id));
+  }
   EXPECT_FALSE(storage.CreateIndex(label1, prop_id));
   EXPECT_THAT(storage.ListAllIndices().label_property,
               UnorderedElementsAre(std::make_pair(label1, prop_id)));
 
   EXPECT_TRUE(storage.CreateIndex(label2, prop_id));
-  EXPECT_TRUE(storage.LabelPropertyIndexExists(label2, prop_id));
+  {
+    auto acc = storage.Access();
+    EXPECT_TRUE(acc.LabelPropertyIndexExists(label2, prop_id));
+  }
   EXPECT_THAT(storage.ListAllIndices().label_property,
               UnorderedElementsAre(std::make_pair(label1, prop_id),
                                    std::make_pair(label2, prop_id)));
 
   EXPECT_TRUE(storage.DropIndex(label1, prop_id));
-  EXPECT_FALSE(storage.LabelPropertyIndexExists(label1, prop_id));
+  {
+    auto acc = storage.Access();
+    EXPECT_FALSE(acc.LabelPropertyIndexExists(label1, prop_id));
+  }
   EXPECT_THAT(storage.ListAllIndices().label_property,
               UnorderedElementsAre(std::make_pair(label2, prop_id)));
   EXPECT_FALSE(storage.DropIndex(label1, prop_id));
 
   EXPECT_TRUE(storage.DropIndex(label2, prop_id));
-  EXPECT_FALSE(storage.LabelPropertyIndexExists(label2, prop_id));
+  {
+    auto acc = storage.Access();
+    EXPECT_FALSE(acc.LabelPropertyIndexExists(label2, prop_id));
+  }
   EXPECT_EQ(storage.ListAllIndices().label_property.size(), 0);
 }