From 34c7a8b098fd8b60ef12ea19d2c62e14be14e00c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ante=20Pu=C5=A1i=C4=87?= Date: Fri, 9 Feb 2024 21:22:53 +0100 Subject: [PATCH] Remove boolean update_text_index from method signatures --- src/dbms/inmemory/replication_handlers.cpp | 6 +-- src/query/db_accessor.hpp | 50 +++++++---------- src/query/plan/operator.cpp | 6 +-- src/query/procedure/mg_procedure_impl.cpp | 14 ++--- src/storage/v2/indices/indices.cpp | 25 +++++---- src/storage/v2/indices/indices.hpp | 7 ++- src/storage/v2/vertex_accessor.cpp | 63 +++++++++++----------- src/storage/v2/vertex_accessor.hpp | 13 +++-- 8 files changed, 83 insertions(+), 101 deletions(-) diff --git a/src/dbms/inmemory/replication_handlers.cpp b/src/dbms/inmemory/replication_handlers.cpp index c9a94c753..b5c3725f9 100644 --- a/src/dbms/inmemory/replication_handlers.cpp +++ b/src/dbms/inmemory/replication_handlers.cpp @@ -535,7 +535,7 @@ uint64_t InMemoryReplicationHandlers::ReadAndApplyDelta(storage::InMemoryStorage if (!vertex) throw utils::BasicException("Invalid transaction! Please raise an issue, {}:{}", __FILE__, __LINE__); // NOTE: Phase 1 of the text search feature doesn't have replication in scope - auto ret = vertex->AddLabel(transaction->NameToLabel(delta.vertex_add_remove_label.label), false); + auto ret = vertex->AddLabel(transaction->NameToLabel(delta.vertex_add_remove_label.label)); if (ret.HasError() || !ret.GetValue()) throw utils::BasicException("Invalid transaction! Please raise an issue, {}:{}", __FILE__, __LINE__); break; @@ -548,7 +548,7 @@ uint64_t InMemoryReplicationHandlers::ReadAndApplyDelta(storage::InMemoryStorage if (!vertex) throw utils::BasicException("Invalid transaction! Please raise an issue, {}:{}", __FILE__, __LINE__); // NOTE: Phase 1 of the text search feature doesn't have replication in scope - auto ret = vertex->RemoveLabel(transaction->NameToLabel(delta.vertex_add_remove_label.label), false); + auto ret = vertex->RemoveLabel(transaction->NameToLabel(delta.vertex_add_remove_label.label)); if (ret.HasError() || !ret.GetValue()) throw utils::BasicException("Invalid transaction! Please raise an issue, {}:{}", __FILE__, __LINE__); break; @@ -562,7 +562,7 @@ uint64_t InMemoryReplicationHandlers::ReadAndApplyDelta(storage::InMemoryStorage throw utils::BasicException("Invalid transaction! Please raise an issue, {}:{}", __FILE__, __LINE__); // NOTE: Phase 1 of the text search feature doesn't have replication in scope auto ret = vertex->SetProperty(transaction->NameToProperty(delta.vertex_edge_set_property.property), - delta.vertex_edge_set_property.value, false); + delta.vertex_edge_set_property.value); if (ret.HasError()) throw utils::BasicException("Invalid transaction! Please raise an issue, {}:{}", __FILE__, __LINE__); break; diff --git a/src/query/db_accessor.hpp b/src/query/db_accessor.hpp index 4086211db..d09eab4df 100644 --- a/src/query/db_accessor.hpp +++ b/src/query/db_accessor.hpp @@ -115,13 +115,9 @@ class VertexAccessor final { auto Labels(storage::View view) const { return impl_.Labels(view); } - storage::Result AddLabel(storage::LabelId label, bool update_text_index = false) { - return impl_.AddLabel(label, update_text_index); - } + storage::Result AddLabel(storage::LabelId label) { return impl_.AddLabel(label); } - storage::Result RemoveLabel(storage::LabelId label, bool update_text_index = false) { - return impl_.RemoveLabel(label, update_text_index); - } + storage::Result RemoveLabel(storage::LabelId label) { return impl_.RemoveLabel(label); } storage::Result HasLabel(storage::View view, storage::LabelId label) const { return impl_.HasLabel(label, view); @@ -133,29 +129,25 @@ class VertexAccessor final { return impl_.GetProperty(key, view); } - storage::Result SetProperty(storage::PropertyId key, const storage::PropertyValue &value, - bool update_text_index = false) { - return impl_.SetProperty(key, value, update_text_index); + storage::Result SetProperty(storage::PropertyId key, const storage::PropertyValue &value) { + return impl_.SetProperty(key, value); } - storage::Result InitProperties(const std::map &properties, - bool update_text_index = false) { - return impl_.InitProperties(properties, update_text_index); + storage::Result InitProperties(const std::map &properties) { + return impl_.InitProperties(properties); } storage::Result>> - UpdateProperties(std::map &properties, - bool update_text_index = false) const { - return impl_.UpdateProperties(properties, update_text_index); + UpdateProperties(std::map &properties) const { + return impl_.UpdateProperties(properties); } - storage::Result RemoveProperty(storage::PropertyId key, bool update_text_index = false) { - return SetProperty(key, storage::PropertyValue(), update_text_index); + storage::Result RemoveProperty(storage::PropertyId key) { + return SetProperty(key, storage::PropertyValue()); } - storage::Result> ClearProperties( - bool update_text_index = false) { - return impl_.ClearProperties(update_text_index); + storage::Result> ClearProperties() { + return impl_.ClearProperties(); } storage::Result InEdges(storage::View view, @@ -262,13 +254,9 @@ class SubgraphVertexAccessor final { auto Labels(storage::View view) const { return impl_.Labels(view); } - storage::Result AddLabel(storage::LabelId label, bool update_text_index = false) { - return impl_.AddLabel(label, update_text_index); - } + storage::Result AddLabel(storage::LabelId label) { return impl_.AddLabel(label); } - storage::Result RemoveLabel(storage::LabelId label, bool update_text_index = false) { - return impl_.RemoveLabel(label, update_text_index); - } + storage::Result RemoveLabel(storage::LabelId label) { return impl_.RemoveLabel(label); } storage::Result HasLabel(storage::View view, storage::LabelId label) const { return impl_.HasLabel(view, label); @@ -286,15 +274,13 @@ class SubgraphVertexAccessor final { storage::Result OutDegree(storage::View view) const { return impl_.OutDegree(view); } - storage::Result SetProperty(storage::PropertyId key, const storage::PropertyValue &value, - bool update_text_index = false) { - return impl_.SetProperty(key, value, update_text_index); + storage::Result SetProperty(storage::PropertyId key, const storage::PropertyValue &value) { + return impl_.SetProperty(key, value); } storage::Result>> - UpdateProperties(std::map &properties, - bool update_text_index = false) const { - return impl_.UpdateProperties(properties, update_text_index); + UpdateProperties(std::map &properties) const { + return impl_.UpdateProperties(properties); } VertexAccessor GetVertexAccessor() const; diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 9bdfe6430..f03ae4e00 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -217,7 +217,7 @@ VertexAccessor &CreateLocalVertex(const NodeCreationInfo &node_info, Frame *fram auto new_node = dba.InsertVertex(); context.execution_stats[ExecutionStats::Key::CREATED_NODES] += 1; for (auto label : node_info.labels) { - auto maybe_error = new_node.AddLabel(label, false); // skip updating text indices until all labels are added + auto maybe_error = new_node.AddLabel(label); // skip updating text indices until all labels are added if (maybe_error.HasError()) { switch (maybe_error.GetError()) { case storage::Error::SERIALIZATION_ERROR: @@ -3177,7 +3177,7 @@ bool SetLabels::SetLabelsCursor::Pull(Frame &frame, ExecutionContext &context) { #endif for (auto label : self_.labels_) { - auto maybe_value = vertex.AddLabel(label, false); // skip updating text indices until all labels are added + auto maybe_value = vertex.AddLabel(label); if (maybe_value.HasError()) { switch (maybe_value.GetError()) { case storage::Error::SERIALIZATION_ERROR: @@ -3345,7 +3345,7 @@ bool RemoveLabels::RemoveLabelsCursor::Pull(Frame &frame, ExecutionContext &cont #endif for (auto label : self_.labels_) { - auto maybe_value = vertex.RemoveLabel(label, false); + auto maybe_value = vertex.RemoveLabel(label); if (maybe_value.HasError()) { switch (maybe_value.GetError()) { case storage::Error::SERIALIZATION_ERROR: diff --git a/src/query/procedure/mg_procedure_impl.cpp b/src/query/procedure/mg_procedure_impl.cpp index d4df39ba7..b0a64bf79 100644 --- a/src/query/procedure/mg_procedure_impl.cpp +++ b/src/query/procedure/mg_procedure_impl.cpp @@ -1841,8 +1841,8 @@ mgp_error mgp_vertex_set_property(struct mgp_vertex *v, const char *property_nam const auto result = std::visit( [prop_key, property_value](auto &impl) { - return impl.SetProperty(prop_key, ToPropertyValue(*property_value), - memgraph::flags::run_time::GetExperimentalTextSearchEnabled()); + // TODO antepusic also update text index + return impl.SetProperty(prop_key, ToPropertyValue(*property_value)); }, v->impl); if (result.HasError()) { @@ -1900,8 +1900,8 @@ mgp_error mgp_vertex_set_properties(struct mgp_vertex *v, struct mgp_map *proper v->graph->impl)); } - const auto result = - v->getImpl().UpdateProperties(props, memgraph::flags::run_time::GetExperimentalTextSearchEnabled()); + const auto result = v->getImpl().UpdateProperties(props); + // TODO antepusic also update text index if (result.HasError()) { switch (result.GetError()) { case memgraph::storage::Error::DELETED_OBJECT: @@ -1960,7 +1960,8 @@ mgp_error mgp_vertex_add_label(struct mgp_vertex *v, mgp_label label) { const auto result = std::visit( [label_id](auto &impl) { - return impl.AddLabel(label_id, memgraph::flags::run_time::GetExperimentalTextSearchEnabled()); + // TODO antepusic also update text index + return impl.AddLabel(label_id); }, v->impl); @@ -2006,7 +2007,8 @@ mgp_error mgp_vertex_remove_label(struct mgp_vertex *v, mgp_label label) { } const auto result = std::visit( [label_id](auto &impl) { - return impl.RemoveLabel(label_id, memgraph::flags::run_time::GetExperimentalTextSearchEnabled()); + // todo also remove from text index + return impl.RemoveLabel(label_id); }, v->impl); diff --git a/src/storage/v2/indices/indices.cpp b/src/storage/v2/indices/indices.cpp index 91a8e0c85..9be2044b3 100644 --- a/src/storage/v2/indices/indices.cpp +++ b/src/storage/v2/indices/indices.cpp @@ -39,29 +39,28 @@ void Indices::RemoveObsoleteEntries(uint64_t oldest_active_start_timestamp, std: ->RemoveObsoleteEntries(oldest_active_start_timestamp, std::move(token)); } -void Indices::UpdateOnAddLabel(LabelId label, Vertex *vertex, const Transaction &tx, Storage *storage, - bool update_text_index) const { +void Indices::UpdateOnAddLabel(LabelId label, Vertex *vertex, const Transaction &tx, Storage *storage) const { label_index_->UpdateOnAddLabel(label, vertex, tx); label_property_index_->UpdateOnAddLabel(label, vertex, tx); - if (update_text_index) { - text_index_.UpdateOnAddLabel(label, vertex, storage->name_id_mapper_.get(), tx.start_timestamp); - } + // if (update_text_index) { + // text_index_.UpdateOnAddLabel(label, vertex, storage->name_id_mapper_.get(), tx.start_timestamp); + // } } -void Indices::UpdateOnRemoveLabel(LabelId label, Vertex *vertex, const Transaction &tx, bool update_text_index) const { +void Indices::UpdateOnRemoveLabel(LabelId label, Vertex *vertex, const Transaction &tx) const { label_index_->UpdateOnRemoveLabel(label, vertex, tx); label_property_index_->UpdateOnRemoveLabel(label, vertex, tx); - if (update_text_index) { - text_index_.UpdateOnRemoveLabel(label, vertex, tx.start_timestamp); - } + // if (update_text_index) { + // text_index_.UpdateOnRemoveLabel(label, vertex, tx.start_timestamp); + // } } void Indices::UpdateOnSetProperty(PropertyId property, const PropertyValue &value, Vertex *vertex, - const Transaction &tx, Storage *storage, bool update_text_index) const { + const Transaction &tx, Storage *storage) const { label_property_index_->UpdateOnSetProperty(property, value, vertex, tx); - if (update_text_index) { - text_index_.UpdateOnSetProperty(vertex, storage->name_id_mapper_.get(), tx.start_timestamp); - } + // if (update_text_index) { + // text_index_.UpdateOnSetProperty(vertex, storage->name_id_mapper_.get(), tx.start_timestamp); + // } } Indices::Indices(const Config &config, StorageMode storage_mode) { diff --git a/src/storage/v2/indices/indices.hpp b/src/storage/v2/indices/indices.hpp index 6b64929aa..71212973a 100644 --- a/src/storage/v2/indices/indices.hpp +++ b/src/storage/v2/indices/indices.hpp @@ -56,15 +56,14 @@ struct Indices { /// This function should be called whenever a label is added to a vertex. /// @throw std::bad_alloc - void UpdateOnAddLabel(LabelId label, Vertex *vertex, const Transaction &tx, Storage *storage, - bool update_text_index = false) const; + void UpdateOnAddLabel(LabelId label, Vertex *vertex, const Transaction &tx, Storage *storage) const; - void UpdateOnRemoveLabel(LabelId label, Vertex *vertex, const Transaction &tx, bool update_text_index = false) const; + void UpdateOnRemoveLabel(LabelId label, Vertex *vertex, const Transaction &tx) const; /// This function should be called whenever a property is modified on a vertex. /// @throw std::bad_alloc void UpdateOnSetProperty(PropertyId property, const PropertyValue &value, Vertex *vertex, const Transaction &tx, - Storage *storage, bool update_text_index = false) const; + Storage *storage) const; std::unique_ptr label_index_; std::unique_ptr label_property_index_; diff --git a/src/storage/v2/vertex_accessor.cpp b/src/storage/v2/vertex_accessor.cpp index d6a87933b..38502152f 100644 --- a/src/storage/v2/vertex_accessor.cpp +++ b/src/storage/v2/vertex_accessor.cpp @@ -100,7 +100,7 @@ bool VertexAccessor::IsVisible(View view) const { return exists && (for_deleted_ || !deleted); } -Result VertexAccessor::AddLabel(LabelId label, bool update_text_index) { +Result VertexAccessor::AddLabel(LabelId label) { if (transaction_->edge_import_mode_active) { throw query::WriteVertexOperationInEdgeImportModeException(); } @@ -124,14 +124,14 @@ Result VertexAccessor::AddLabel(LabelId label, bool update_text_index) { /// TODO: some by pointers, some by reference => not good, make it better storage_->constraints_.unique_constraints_->UpdateOnAddLabel(label, *vertex_, transaction_->start_timestamp); transaction_->constraint_verification_info.AddedLabel(vertex_); - storage_->indices_.UpdateOnAddLabel(label, vertex_, *transaction_, storage_, update_text_index); + storage_->indices_.UpdateOnAddLabel(label, vertex_, *transaction_, storage_); transaction_->manyDeltasCache.Invalidate(vertex_, label); return true; } /// TODO: move to after update and change naming to vertex after update -Result VertexAccessor::RemoveLabel(LabelId label, bool update_text_index) { +Result VertexAccessor::RemoveLabel(LabelId label) { if (transaction_->edge_import_mode_active) { throw query::WriteVertexOperationInEdgeImportModeException(); } @@ -152,7 +152,7 @@ Result VertexAccessor::RemoveLabel(LabelId label, bool update_text_index) /// TODO: some by pointers, some by reference => not good, make it better storage_->constraints_.unique_constraints_->UpdateOnRemoveLabel(label, *vertex_, transaction_->start_timestamp); - storage_->indices_.UpdateOnRemoveLabel(label, vertex_, *transaction_, update_text_index); + storage_->indices_.UpdateOnRemoveLabel(label, vertex_, *transaction_); transaction_->manyDeltasCache.Invalidate(vertex_, label); return true; @@ -252,8 +252,7 @@ Result> VertexAccessor::Labels(View view) const { return std::move(labels); } -Result VertexAccessor::SetProperty(PropertyId property, const PropertyValue &value, - bool update_text_index) { +Result VertexAccessor::SetProperty(PropertyId property, const PropertyValue &value) { if (transaction_->edge_import_mode_active) { throw query::WriteVertexOperationInEdgeImportModeException(); } @@ -285,14 +284,13 @@ Result VertexAccessor::SetProperty(PropertyId property, const Pro } else { transaction_->constraint_verification_info.RemovedProperty(vertex_); } - storage_->indices_.UpdateOnSetProperty(property, value, vertex_, *transaction_, storage_, update_text_index); + storage_->indices_.UpdateOnSetProperty(property, value, vertex_, *transaction_, storage_); transaction_->manyDeltasCache.Invalidate(vertex_, property); return std::move(current_value); } -Result VertexAccessor::InitProperties(const std::map &properties, - bool update_text_index) { +Result VertexAccessor::InitProperties(const std::map &properties) { if (transaction_->edge_import_mode_active) { throw query::WriteVertexOperationInEdgeImportModeException(); } @@ -305,14 +303,14 @@ Result VertexAccessor::InitProperties(const std::mapdeleted) return Error::DELETED_OBJECT; bool result{false}; utils::AtomicMemoryBlock atomic_memory_block{ - [&result, &properties, storage = storage_, transaction = transaction_, vertex = vertex_, update_text_index]() { + [&result, &properties, storage = storage_, transaction = transaction_, vertex = vertex_]() { if (!vertex->properties.InitProperties(properties)) { result = false; return; } for (const auto &[property, value] : properties) { CreateAndLinkDelta(transaction, vertex, Delta::SetPropertyTag(), property, PropertyValue()); - storage->indices_.UpdateOnSetProperty(property, value, vertex, *transaction, storage, update_text_index); + storage->indices_.UpdateOnSetProperty(property, value, vertex, *transaction, storage); transaction->manyDeltasCache.Invalidate(vertex, property); if (!value.IsNull()) { transaction->constraint_verification_info.AddedProperty(vertex); @@ -328,7 +326,7 @@ Result VertexAccessor::InitProperties(const std::map>> VertexAccessor::UpdateProperties( - std::map &properties, bool update_text_index) const { + std::map &properties) const { if (transaction_->edge_import_mode_active) { throw query::WriteVertexOperationInEdgeImportModeException(); } @@ -342,30 +340,30 @@ Result>> Vertex using ReturnType = decltype(vertex_->properties.UpdateProperties(properties)); std::optional id_old_new_change; - utils::AtomicMemoryBlock atomic_memory_block{[storage = storage_, transaction = transaction_, vertex = vertex_, - &properties, &id_old_new_change, update_text_index]() { - id_old_new_change.emplace(vertex->properties.UpdateProperties(properties)); + utils::AtomicMemoryBlock atomic_memory_block{ + [storage = storage_, transaction = transaction_, vertex = vertex_, &properties, &id_old_new_change]() { + id_old_new_change.emplace(vertex->properties.UpdateProperties(properties)); - if (!id_old_new_change.has_value()) { - return; - } - for (auto &[id, old_value, new_value] : *id_old_new_change) { - storage->indices_.UpdateOnSetProperty(id, new_value, vertex, *transaction, storage, update_text_index); - CreateAndLinkDelta(transaction, vertex, Delta::SetPropertyTag(), id, std::move(old_value)); - transaction->manyDeltasCache.Invalidate(vertex, id); - if (!new_value.IsNull()) { - transaction->constraint_verification_info.AddedProperty(vertex); - } else { - transaction->constraint_verification_info.RemovedProperty(vertex); - } - } - }}; + if (!id_old_new_change.has_value()) { + return; + } + for (auto &[id, old_value, new_value] : *id_old_new_change) { + storage->indices_.UpdateOnSetProperty(id, new_value, vertex, *transaction, storage); + CreateAndLinkDelta(transaction, vertex, Delta::SetPropertyTag(), id, std::move(old_value)); + transaction->manyDeltasCache.Invalidate(vertex, id); + if (!new_value.IsNull()) { + transaction->constraint_verification_info.AddedProperty(vertex); + } else { + transaction->constraint_verification_info.RemovedProperty(vertex); + } + } + }}; std::invoke(atomic_memory_block); return id_old_new_change.has_value() ? std::move(id_old_new_change.value()) : ReturnType{}; } -Result> VertexAccessor::ClearProperties(bool update_text_index) { +Result> VertexAccessor::ClearProperties() { if (transaction_->edge_import_mode_active) { throw query::WriteVertexOperationInEdgeImportModeException(); } @@ -378,15 +376,14 @@ Result> VertexAccessor::ClearProperties(bool using ReturnType = decltype(vertex_->properties.Properties()); std::optional properties; utils::AtomicMemoryBlock atomic_memory_block{ - [storage = storage_, transaction = transaction_, vertex = vertex_, &properties, update_text_index]() { + [storage = storage_, transaction = transaction_, vertex = vertex_, &properties]() { properties.emplace(vertex->properties.Properties()); if (!properties.has_value()) { return; } for (const auto &[property, value] : *properties) { CreateAndLinkDelta(transaction, vertex, Delta::SetPropertyTag(), property, value); - storage->indices_.UpdateOnSetProperty(property, PropertyValue(), vertex, *transaction, storage, - update_text_index); + storage->indices_.UpdateOnSetProperty(property, PropertyValue(), vertex, *transaction, storage); transaction->constraint_verification_info.RemovedProperty(vertex); transaction->manyDeltasCache.Invalidate(vertex, property); } diff --git a/src/storage/v2/vertex_accessor.hpp b/src/storage/v2/vertex_accessor.hpp index 6567b5fdb..44b31fb48 100644 --- a/src/storage/v2/vertex_accessor.hpp +++ b/src/storage/v2/vertex_accessor.hpp @@ -47,12 +47,12 @@ class VertexAccessor final { /// Add a label and return `true` if insertion took place. /// `false` is returned if the label already existed. /// @throw std::bad_alloc - Result AddLabel(LabelId label, bool update_text_index = false); + Result AddLabel(LabelId label); /// Remove a label and return `true` if deletion took place. /// `false` is returned if the vertex did not have a label already. /// @throw std::bad_alloc - Result RemoveLabel(LabelId label, bool update_text_index = false); + Result RemoveLabel(LabelId label); Result HasLabel(LabelId label, View view) const; @@ -63,20 +63,19 @@ class VertexAccessor final { /// Set a property value and return the old value. /// @throw std::bad_alloc - Result SetProperty(PropertyId property, const PropertyValue &value, bool update_text_index = false); + Result SetProperty(PropertyId property, const PropertyValue &value); /// Set property values only if property store is empty. Returns `true` if successully set all values, /// `false` otherwise. /// @throw std::bad_alloc - Result InitProperties(const std::map &properties, - bool update_text_index = false); + Result InitProperties(const std::map &properties); Result>> UpdateProperties( - std::map &properties, bool update_text_index = false) const; + std::map &properties) const; /// Remove all properties and return the values of the removed properties. /// @throw std::bad_alloc - Result> ClearProperties(bool update_text_index = false); + Result> ClearProperties(); /// @throw std::bad_alloc Result GetProperty(PropertyId property, View view) const;