diff --git a/src/query/typed_value.cpp b/src/query/typed_value.cpp index 6719f52f6..13db88e1c 100644 --- a/src/query/typed_value.cpp +++ b/src/query/typed_value.cpp @@ -22,6 +22,7 @@ #include "storage/v2/temporal.hpp" #include "utils/exceptions.hpp" #include "utils/fnv.hpp" +#include "utils/memory.hpp" namespace memgraph::query { @@ -215,7 +216,8 @@ TypedValue::TypedValue(const TypedValue &other, utils::MemoryResource *memory) : new (&duration_v) utils::Duration(other.duration_v); return; case Type::Graph: - new (&graph_v) Graph(other.graph_v, memory_); + auto *graph_ptr = utils::Allocator(memory_).new_object(*other.graph_v); + new (&graph_v) std::unique_ptr(graph_ptr); return; } LOG_FATAL("Unsupported TypedValue::Type"); @@ -267,7 +269,12 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory) : memo new (&duration_v) utils::Duration(other.duration_v); break; case Type::Graph: - new (&graph_v) Graph(std::move(other.graph_v), memory_); + if (other.GetMemoryResource() == memory_) { + new (&graph_v) std::unique_ptr(std::move(other.graph_v)); + } else { + auto *graph_ptr = utils::Allocator(memory_).new_object(std::move(*other.graph_v)); + new (&graph_v) std::unique_ptr(graph_ptr); + } } other.DestroyValue(); } @@ -336,7 +343,22 @@ DEFINE_VALUE_AND_TYPE_GETTERS(utils::Date, Date, date_v) DEFINE_VALUE_AND_TYPE_GETTERS(utils::LocalTime, LocalTime, local_time_v) DEFINE_VALUE_AND_TYPE_GETTERS(utils::LocalDateTime, LocalDateTime, local_date_time_v) DEFINE_VALUE_AND_TYPE_GETTERS(utils::Duration, Duration, duration_v) -DEFINE_VALUE_AND_TYPE_GETTERS(Graph, Graph, graph_v) + +Graph &TypedValue::ValueGraph() { + if (type_ != Type::Graph) { + throw TypedValueException("TypedValue is of type '{}', not '{}'", type_, Type::Graph); + } + return *graph_v; +} + +const Graph &TypedValue::ValueGraph() const { + if (type_ != Type::Graph) { + throw TypedValueException("TypedValue is of type '{}', not '{}'", type_, Type::Graph); + } + return *graph_v; +} + +bool TypedValue::IsGraph() const { return type_ == Type::Graph; } #undef DEFINE_VALUE_AND_TYPE_GETTERS @@ -530,9 +552,11 @@ TypedValue &TypedValue::operator=(const TypedValue &other) { case TypedValue::Type::Path: new (&path_v) Path(other.path_v, memory_); return *this; - case TypedValue::Type::Graph: - new (&graph_v) Graph(other.graph_v, memory_); + case TypedValue::Type::Graph: { + auto *graph_ptr = utils::Allocator(memory_).new_object(*other.graph_v); + new (&graph_v) std::unique_ptr(graph_ptr); return *this; + } case Type::Date: new (&date_v) utils::Date(other.date_v); return *this; @@ -605,7 +629,12 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept(false) { new (&duration_v) utils::Duration(other.duration_v); break; case Type::Graph: - new (&graph_v) Graph(std::move(other.graph_v), memory_); + if (other.GetMemoryResource() == memory_) { + new (&graph_v) std::unique_ptr(std::move(other.graph_v)); + } else { + auto *graph_ptr = utils::Allocator(memory_).new_object(std::move(*other.graph_v)); + new (&graph_v) std::unique_ptr(graph_ptr); + } break; } other.DestroyValue(); @@ -625,31 +654,36 @@ void TypedValue::DestroyValue() { // we need to call destructors for non primitive types since we used // placement new case Type::String: - string_v.~TString(); + std::destroy_at(&string_v); break; case Type::List: - list_v.~TVector(); + std::destroy_at(&list_v); break; case Type::Map: - map_v.~TMap(); + std::destroy_at(&map_v); break; case Type::Vertex: - vertex_v.~VertexAccessor(); + std::destroy_at(&vertex_v); break; case Type::Edge: - edge_v.~EdgeAccessor(); + std::destroy_at(&edge_v); break; case Type::Path: - path_v.~Path(); + std::destroy_at(&path_v); break; case Type::Date: case Type::LocalTime: case Type::LocalDateTime: case Type::Duration: break; - case Type::Graph: - graph_v.~Graph(); + case Type::Graph: { + auto *graph = graph_v.release(); + std::destroy_at(&graph_v); + if (graph) { + utils::Allocator(memory_).delete_object(graph); + } break; + } } type_ = TypedValue::Type::Null; diff --git a/src/query/typed_value.hpp b/src/query/typed_value.hpp index b0eadf70e..bebda5ce4 100644 --- a/src/query/typed_value.hpp +++ b/src/query/typed_value.hpp @@ -416,7 +416,8 @@ class TypedValue { * element-wise move and graph is not guaranteed to be empty. */ TypedValue(Graph &&graph, utils::MemoryResource *memory) : memory_(memory), type_(Type::Graph) { - new (&graph_v) Graph(std::move(graph), memory_); + auto *graph_ptr = utils::Allocator(memory_).new_object(std::move(graph)); + new (&graph_v) std::unique_ptr(graph_ptr); } /** @@ -547,7 +548,8 @@ class TypedValue { utils::LocalTime local_time_v; utils::LocalDateTime local_date_time_v; utils::Duration duration_v; - Graph graph_v; + // As the unique_ptr is not allocator aware, it requires special attention when copying or moving graphs + std::unique_ptr graph_v; }; /** diff --git a/tests/stress/long_running.cpp b/tests/stress/long_running.cpp index ddf93549d..f266cf614 100644 --- a/tests/stress/long_running.cpp +++ b/tests/stress/long_running.cpp @@ -254,6 +254,22 @@ class GraphSession { Execute(fmt::format("MATCH ()-[e]->() WHERE e.id > {} AND e.id < {} SET e.value = {}", lo, hi, num)); } + void CheckGraphProjection() { + uint64_t vertex_id = *vertices_.rbegin(); + uint64_t lo = std::floor(GetRandom() * vertex_id); + uint64_t hi = std::floor(lo + vertex_id * 0.01); + + Execute(fmt::format( + "MATCH p=()-[e]->() WHERE e.id > {} AND e.id < {} WITH project(p) as graph WITH graph.nodes as nodes " + "UNWIND nodes as n RETURN n.x " + "as x ORDER BY x DESC", + lo, hi)); + Execute(fmt::format( + "MATCH p=()-[e]->() WHERE e.id > {} AND e.id < {} WITH project(p) as graph WITH graph.edges as edges " + "UNWIND edges as e RETURN e.prop as y ORDER BY y DESC", + lo, hi)); + } + /** Checks if the local info corresponds to DB state */ void VerifyGraph() { // helper lambda for set verification @@ -357,6 +373,7 @@ class GraphSession { } else { CreateVertices(1); } + CheckGraphProjection(); } // final verification diff --git a/tests/unit/typed_value.cpp b/tests/unit/typed_value.cpp index 1eba51dbb..0238363b6 100644 --- a/tests/unit/typed_value.cpp +++ b/tests/unit/typed_value.cpp @@ -20,6 +20,7 @@ #include "gtest/gtest.h" +#include "query/graph.hpp" #include "query/typed_value.hpp" #include "storage/v2/storage.hpp" @@ -48,8 +49,13 @@ class AllTypesFixture : public testing::Test { {"e", TypedValue()}}); auto vertex = dba.InsertVertex(); values_.emplace_back(vertex); - values_.emplace_back(*dba.InsertEdge(&vertex, &vertex, dba.NameToEdgeType("et"))); + auto edge = *dba.InsertEdge(&vertex, &vertex, dba.NameToEdgeType("et")); + values_.emplace_back(edge); values_.emplace_back(memgraph::query::Path(dba.InsertVertex())); + memgraph::query::Graph graph{memgraph::utils::NewDeleteResource()}; + graph.InsertVertex(vertex); + graph.InsertEdge(edge); + values_.emplace_back(std::move(graph)); } }; @@ -533,6 +539,18 @@ TEST_F(AllTypesFixture, PropagationOfMemoryOnConstruction) { EXPECT_EQ(copied.vertices(), original.vertices()); EXPECT_EQ(copied.edges(), original.edges()); EXPECT_EQ(copied.GetMemoryResource(), &monotonic_memory); + } else if (value.type() == TypedValue::Type::Graph) { + ASSERT_EQ(move_constructed_value.type(), value.type()); + const auto &original = value.ValueGraph(); + const auto &moved = move_constructed_value.ValueGraph(); + const auto &copied = copy_constructed_value.ValueGraph(); + EXPECT_EQ(original.GetMemoryResource(), memgraph::utils::NewDeleteResource()); + EXPECT_EQ(moved.vertices(), original.vertices()); + EXPECT_EQ(moved.edges(), original.edges()); + EXPECT_EQ(moved.GetMemoryResource(), &monotonic_memory); + EXPECT_EQ(copied.vertices(), original.vertices()); + EXPECT_EQ(copied.edges(), original.edges()); + EXPECT_EQ(copied.GetMemoryResource(), &monotonic_memory); } } }