From 89f42ef73eeca486f97137db125fccd8b23e228a Mon Sep 17 00:00:00 2001 From: Kostas Kyrimis Date: Wed, 7 Dec 2022 18:33:47 +0200 Subject: [PATCH 1/9] Add CreateExpand PullMultiple and prototype mocks for testing --- src/query/v2/multiframe.cpp | 2 +- src/query/v2/multiframe.hpp | 2 +- src/query/v2/plan/operator.cpp | 74 ++++++++++++++++ tests/unit/CMakeLists.txt | 4 + tests/unit/mock_helpers.hpp | 63 +++++++++++++ .../query_v2_create_expand_multiframe.cpp | 88 +++++++++++++++++++ 6 files changed, 231 insertions(+), 2 deletions(-) create mode 100644 tests/unit/mock_helpers.hpp create mode 100644 tests/unit/query_v2_create_expand_multiframe.cpp diff --git a/src/query/v2/multiframe.cpp b/src/query/v2/multiframe.cpp index 4829addb2..88262e514 100644 --- a/src/query/v2/multiframe.cpp +++ b/src/query/v2/multiframe.cpp @@ -24,7 +24,7 @@ static_assert(std::forward_iterator); static_assert(std::forward_iterator); static_assert(std::forward_iterator); -MultiFrame::MultiFrame(int64_t size_of_frame, size_t number_of_frames, utils::MemoryResource *execution_memory) +MultiFrame::MultiFrame(size_t size_of_frame, size_t number_of_frames, utils::MemoryResource *execution_memory) : frames_(utils::pmr::vector( number_of_frames, FrameWithValidity(size_of_frame, execution_memory), execution_memory)) { MG_ASSERT(number_of_frames > 0); diff --git a/src/query/v2/multiframe.hpp b/src/query/v2/multiframe.hpp index 0b6896422..37e6fea11 100644 --- a/src/query/v2/multiframe.hpp +++ b/src/query/v2/multiframe.hpp @@ -30,7 +30,7 @@ class MultiFrame { friend class ValidFramesReader; friend class InvalidFramesPopulator; - MultiFrame(int64_t size_of_frame, size_t number_of_frames, utils::MemoryResource *execution_memory); + MultiFrame(size_t size_of_frame, size_t number_of_frames, utils::MemoryResource *execution_memory); ~MultiFrame() = default; MultiFrame(const MultiFrame &other); diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index eeb5cd6b4..c909cc466 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -2376,6 +2376,22 @@ class DistributedCreateExpandCursor : public Cursor { return true; } + void PullMultiple(MultiFrame &multi_frame, ExecutionContext &context) override { + SCOPED_PROFILE_OP("CreateExpandMF"); + input_cursor_->PullMultiple(multi_frame, context); + auto request_vertices = ExpandCreationInfoToRequests(multi_frame, context); + { + SCOPED_REQUEST_WAIT_PROFILE; + auto &request_router = context.request_router; + auto results = request_router->CreateExpand(std::move(request_vertices)); + for (const auto &result : results) { + if (result.error) { + throw std::runtime_error("CreateExpand Request failed"); + } + } + } + } + void Shutdown() override { input_cursor_->Shutdown(); } void Reset() override { @@ -2450,6 +2466,64 @@ class DistributedCreateExpandCursor : public Cursor { return edge_requests; } + std::vector ExpandCreationInfoToRequests(MultiFrame &multi_frame, ExecutionContext &context) const { + std::vector edge_requests; + auto reader = multi_frame.GetValidFramesConsumer(); + + for (auto &frame : reader) { + const auto &edge_info = self_.edge_info_; + msgs::NewExpand request{.id = {context.edge_ids_alloc->AllocateId()}}; + ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, nullptr, + storage::v3::View::NEW); + request.type = {edge_info.edge_type}; + if (const auto *edge_info_properties = std::get_if(&edge_info.properties)) { + for (const auto &[property, value_expression] : *edge_info_properties) { + TypedValue val = value_expression->Accept(evaluator); + request.properties.emplace_back(property, storage::v3::TypedValueToValue(val)); + } + } else { + // handle parameter + auto property_map = evaluator.Visit(*std::get(edge_info.properties)).ValueMap(); + for (const auto &[property, value] : property_map) { + const auto property_id = context.request_router->NameToProperty(std::string(property)); + request.properties.emplace_back(property_id, storage::v3::TypedValueToValue(value)); + } + } + // src, dest + TypedValue &v1_value = frame[self_.input_symbol_]; + const auto &v1 = v1_value.ValueVertex(); + const auto &v2 = OtherVertex(frame); + + // Set src and dest vertices + // TODO(jbajic) Currently we are only handling scenario where vertices + // are matched + const auto set_vertex = [](const auto &vertex, auto &vertex_id) { + vertex_id.first = vertex.PrimaryLabel(); + vertex_id.second = vertex.GetVertex().id.second; + }; + + std::invoke([&]() { + switch (edge_info.direction) { + case EdgeAtom::Direction::IN: { + set_vertex(v2, request.src_vertex); + set_vertex(v1, request.dest_vertex); + break; + } + case EdgeAtom::Direction::OUT: { + set_vertex(v1, request.src_vertex); + set_vertex(v2, request.dest_vertex); + break; + } + case EdgeAtom::Direction::BOTH: + LOG_FATAL("Must indicate exact expansion direction here"); + } + }); + + edge_requests.push_back(std::move(request)); + } + return edge_requests; + } + private: void ResetExecutionState() {} diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 188c6c1b0..b1c5c9c6f 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -410,3 +410,7 @@ target_link_libraries(${test_prefix}high_density_shard_create_scan mg-io mg-coor # Tests for awesome_memgraph_functions add_unit_test(query_v2_expression_evaluator.cpp) target_link_libraries(${test_prefix}query_v2_expression_evaluator mg-query-v2) + +# Tests for multiframes +add_unit_test(query_v2_create_expand_multiframe.cpp) +target_link_libraries(${test_prefix}query_v2_create_expand_multiframe mg-query-v2) diff --git a/tests/unit/mock_helpers.hpp b/tests/unit/mock_helpers.hpp new file mode 100644 index 000000000..5201aa210 --- /dev/null +++ b/tests/unit/mock_helpers.hpp @@ -0,0 +1,63 @@ +// Copyright 2022 Memgraph Ltd. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source +// License, and you may not use this file except in compliance with the Business Source License. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +#pragma once + +#include +#include +#include "query/v2/plan/operator.hpp" +#include "query/v2/request_router.hpp" + +namespace memgraph { +class MockedRequestRouter : public query::v2::RequestRouterInterface { + public: + MOCK_METHOD1(ScanVertices, std::vector(std::optional label)); + MOCK_METHOD1(CreateVertices, std::vector(std::vector)); + MOCK_METHOD1(ExpandOne, std::vector(msgs::ExpandOneRequest)); + MOCK_METHOD1(CreateExpand, std::vector(std::vector)); + MOCK_METHOD1(GetProperties, std::vector(msgs::GetPropertiesRequest)); + MOCK_METHOD0(StartTransaction, void()); + MOCK_METHOD0(Commit, void()); + + MOCK_CONST_METHOD1(NameToEdgeType, storage::v3::EdgeTypeId(const std::string &)); + MOCK_CONST_METHOD1(NameToProperty, storage::v3::PropertyId(const std::string &)); + MOCK_CONST_METHOD1(NameToLabel, storage::v3::LabelId(const std::string &)); + MOCK_CONST_METHOD1(LabelToName, storage::v3::LabelId(const std::string &)); + MOCK_CONST_METHOD1(PropertyToName, const std::string &(storage::v3::PropertyId)); + MOCK_CONST_METHOD1(LabelToName, const std::string &(storage::v3::LabelId label)); + MOCK_CONST_METHOD1(EdgeTypeToName, const std::string &(storage::v3::EdgeTypeId type)); + MOCK_CONST_METHOD1(MaybeNameToProperty, std::optional(const std::string &)); + MOCK_CONST_METHOD1(MaybeNameToEdgeType, std::optional(const std::string &)); + MOCK_CONST_METHOD1(MaybeNameToLabel, std::optional(const std::string &)); + MOCK_CONST_METHOD1(IsPrimaryLabel, bool(storage::v3::LabelId)); + MOCK_CONST_METHOD2(IsPrimaryKey, bool(storage::v3::LabelId, storage::v3::PropertyId)); +}; + +class MockedLogicalOperator : query::v2::plan::LogicalOperator { + public: + MOCK_CONST_METHOD1(MakeCursor, query::v2::plan::UniqueCursorPtr(utils::MemoryResource *)); + MOCK_CONST_METHOD1(OutputSymbols, std::vector(const expr::SymbolTable &)); + MOCK_CONST_METHOD1(ModifiedSymbols, std::vector(const expr::SymbolTable &)); + MOCK_CONST_METHOD0(HasSingleInput, bool()); + MOCK_CONST_METHOD0(input, std::shared_ptr()); + MOCK_METHOD1(set_input, void(std::shared_ptr)); + MOCK_CONST_METHOD1(Clone, std::unique_ptr(query::v2::AstStorage *storage)); +}; + +class MockedCursor : memgraph::query::v2::plan::Cursor { + public: + MOCK_METHOD2(Pull, bool(query::v2::Frame &, expr::ExecutionContext &)); + MOCK_METHOD2(PullMultiple, void(query::v2::MultiFrame &, expr::ExecutionContext &)); + MOCK_METHOD0(Reset, void()); + MOCK_METHOD0(Shutdown, void()); +}; + +} // namespace memgraph diff --git a/tests/unit/query_v2_create_expand_multiframe.cpp b/tests/unit/query_v2_create_expand_multiframe.cpp new file mode 100644 index 000000000..f6f6567db --- /dev/null +++ b/tests/unit/query_v2_create_expand_multiframe.cpp @@ -0,0 +1,88 @@ +// Copyright 2022 Memgraph Ltd. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source +// License, and you may not use this file except in compliance with the Business Source License. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +#include "mock_helpers.hpp" + +#include "query/v2/bindings/frame.hpp" +#include "query/v2/bindings/symbol_table.hpp" +#include "query/v2/common.hpp" +#include "query/v2/context.hpp" +#include "query/v2/plan/operator.hpp" +#include "query/v2/requests.hpp" +#include "storage/v3/property_value.hpp" +#include "storage/v3/shard.hpp" +#include "utils/logging.hpp" +#include "utils/memory.hpp" + +using namespace memgraph::query::v2; +using namespace memgraph::query::v2::plan; +namespace memgraph { +class TestTemplate : public testing::Test { + protected: + void SetUp() override {} +}; + +ExecutionContext MakeContext(const AstStorage &storage, const SymbolTable &symbol_table, RequestRouterInterface *router, + IdAllocator *id_alloc) { + ExecutionContext context; + context.symbol_table = symbol_table; + context.evaluation_context.properties = NamesToProperties(storage.properties_, router); + context.evaluation_context.labels = NamesToLabels(storage.labels_, router); + context.edge_ids_alloc = id_alloc; + context.request_router = router; + return context; +} + +MultiFrame CreateMultiFrame(const size_t max_pos, const Symbol &src, const Symbol &dst, MockedRequestRouter *router) { + static constexpr size_t frame_size = 100; + MultiFrame multi_frame(max_pos, frame_size, utils::NewDeleteResource()); + auto frames_populator = multi_frame.GetInvalidFramesPopulator(); + size_t i = 0; + for (auto &frame : frames_populator) { + frame.MakeValid(); + auto &src_acc = frame.at(src); + auto &dst_acc = frame.at(dst); + auto v1 = msgs::Vertex{.id = {{msgs::LabelId::FromUint(1)}, {msgs::Value(static_cast(i++))}}}; + auto v2 = msgs::Vertex{.id = {{msgs::LabelId::FromUint(1)}, {msgs::Value(static_cast(i++))}}}; + std::map mp; + src_acc = TypedValue(query::v2::accessors::VertexAccessor(v1, mp, router)); + dst_acc = TypedValue(query::v2::accessors::VertexAccessor(v2, mp, router)); + } + + return multi_frame; +} + +TEST_F(TestTemplate, CreateExpand) { + MockedRequestRouter router; + + AstStorage ast; + SymbolTable symbol_table; + + query::v2::plan::NodeCreationInfo node; + query::v2::plan::EdgeCreationInfo edge; + edge.edge_type = msgs::EdgeTypeId::FromUint(1); + edge.direction = EdgeAtom::Direction::IN; + auto id_alloc = IdAllocator(0, 100); + + const auto &src = symbol_table.CreateSymbol("n", true); + node.symbol = symbol_table.CreateSymbol("u", true); + + auto create_expand = query::v2::plan::CreateExpand(node, edge, nullptr, src, true); + auto cursor = create_expand.MakeCursor(utils::NewDeleteResource()); + + EXPECT_CALL(router, CreateExpand(testing::_)) + .Times(1) + .WillOnce(::testing::Return(std::vector{})); + auto context = MakeContext(ast, symbol_table, &router, &id_alloc); + auto multi_frame = CreateMultiFrame(context.symbol_table.max_position(), src, node.symbol, &router); + cursor->PullMultiple(multi_frame, context); +} +} // namespace memgraph From 2e4e975102ecedb8f73a49b09c3fd336b1f1cd7c Mon Sep 17 00:00:00 2001 From: Kostas Kyrimis Date: Mon, 12 Dec 2022 19:15:28 +0200 Subject: [PATCH 2/9] Update GoogleTest lib to latest release version 1.12.1 --- libs/setup.sh | 2 +- tests/unit/utils_settings.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/setup.sh b/libs/setup.sh index bb737d432..55288f535 100755 --- a/libs/setup.sh +++ b/libs/setup.sh @@ -171,7 +171,7 @@ benchmark_tag="v1.6.0" repo_clone_try_double "${primary_urls[gbenchmark]}" "${secondary_urls[gbenchmark]}" "benchmark" "$benchmark_tag" true # google test -googletest_tag="release-1.8.0" +googletest_tag="release-1.12.1" repo_clone_try_double "${primary_urls[gtest]}" "${secondary_urls[gtest]}" "googletest" "$googletest_tag" true # libbcrypt diff --git a/tests/unit/utils_settings.cpp b/tests/unit/utils_settings.cpp index 388e467ed..20262e1f5 100644 --- a/tests/unit/utils_settings.cpp +++ b/tests/unit/utils_settings.cpp @@ -11,7 +11,7 @@ #include -#include +#include #include #include "utils/settings.hpp" From f04ed3c1372d15d1b0d10e4f1cd3f2c756317a9c Mon Sep 17 00:00:00 2001 From: Kostas Kyrimis Date: Mon, 12 Dec 2022 19:15:49 +0200 Subject: [PATCH 3/9] Simplify Mocks and test --- src/query/v2/plan/operator.cpp | 2 +- tests/unit/mock_helpers.hpp | 89 +++++++++++-------- .../query_v2_create_expand_multiframe.cpp | 45 ++++------ 3 files changed, 73 insertions(+), 63 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index c909cc466..012981e44 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -2468,7 +2468,7 @@ class DistributedCreateExpandCursor : public Cursor { std::vector ExpandCreationInfoToRequests(MultiFrame &multi_frame, ExecutionContext &context) const { std::vector edge_requests; - auto reader = multi_frame.GetValidFramesConsumer(); + auto reader = multi_frame.GetValidFramesModifier(); for (auto &frame : reader) { const auto &edge_info = self_.edge_info_; diff --git a/tests/unit/mock_helpers.hpp b/tests/unit/mock_helpers.hpp index 5201aa210..0010d5986 100644 --- a/tests/unit/mock_helpers.hpp +++ b/tests/unit/mock_helpers.hpp @@ -13,51 +13,70 @@ #include #include +#include "query/v2/common.hpp" +#include "query/v2/context.hpp" #include "query/v2/plan/operator.hpp" #include "query/v2/request_router.hpp" -namespace memgraph { -class MockedRequestRouter : public query::v2::RequestRouterInterface { +namespace memgraph::query::v2 { +class MockedRequestRouter : public RequestRouterInterface { public: - MOCK_METHOD1(ScanVertices, std::vector(std::optional label)); - MOCK_METHOD1(CreateVertices, std::vector(std::vector)); - MOCK_METHOD1(ExpandOne, std::vector(msgs::ExpandOneRequest)); - MOCK_METHOD1(CreateExpand, std::vector(std::vector)); - MOCK_METHOD1(GetProperties, std::vector(msgs::GetPropertiesRequest)); - MOCK_METHOD0(StartTransaction, void()); - MOCK_METHOD0(Commit, void()); + MOCK_METHOD(std::vector, ScanVertices, (std::optional label)); + MOCK_METHOD(std::vector, CreateVertices, (std::vector)); + MOCK_METHOD(std::vector, ExpandOne, (msgs::ExpandOneRequest)); + MOCK_METHOD(std::vector, CreateExpand, (std::vector)); + MOCK_METHOD(std::vector, GetProperties, (msgs::GetPropertiesRequest)); + MOCK_METHOD(void, StartTransaction, ()); + MOCK_METHOD(void, Commit, ()); - MOCK_CONST_METHOD1(NameToEdgeType, storage::v3::EdgeTypeId(const std::string &)); - MOCK_CONST_METHOD1(NameToProperty, storage::v3::PropertyId(const std::string &)); - MOCK_CONST_METHOD1(NameToLabel, storage::v3::LabelId(const std::string &)); - MOCK_CONST_METHOD1(LabelToName, storage::v3::LabelId(const std::string &)); - MOCK_CONST_METHOD1(PropertyToName, const std::string &(storage::v3::PropertyId)); - MOCK_CONST_METHOD1(LabelToName, const std::string &(storage::v3::LabelId label)); - MOCK_CONST_METHOD1(EdgeTypeToName, const std::string &(storage::v3::EdgeTypeId type)); - MOCK_CONST_METHOD1(MaybeNameToProperty, std::optional(const std::string &)); - MOCK_CONST_METHOD1(MaybeNameToEdgeType, std::optional(const std::string &)); - MOCK_CONST_METHOD1(MaybeNameToLabel, std::optional(const std::string &)); - MOCK_CONST_METHOD1(IsPrimaryLabel, bool(storage::v3::LabelId)); - MOCK_CONST_METHOD2(IsPrimaryKey, bool(storage::v3::LabelId, storage::v3::PropertyId)); + MOCK_METHOD(storage::v3::EdgeTypeId, NameToEdgeType, (const std::string &), (const)); + MOCK_METHOD(storage::v3::PropertyId, NameToProperty, (const std::string &), (const)); + MOCK_METHOD(storage::v3::LabelId, NameToLabel, (const std::string &), (const)); + MOCK_METHOD(storage::v3::LabelId, LabelToName, (const std::string &), (const)); + MOCK_METHOD(const std::string &, PropertyToName, (storage::v3::PropertyId), (const)); + MOCK_METHOD(const std::string &, LabelToName, (storage::v3::LabelId label), (const)); + MOCK_METHOD(const std::string &, EdgeTypeToName, (storage::v3::EdgeTypeId type), (const)); + MOCK_METHOD(std::optional, MaybeNameToProperty, (const std::string &), (const)); + MOCK_METHOD(std::optional, MaybeNameToEdgeType, (const std::string &), (const)); + MOCK_METHOD(std::optional, MaybeNameToLabel, (const std::string &), (const)); + MOCK_METHOD(bool, IsPrimaryLabel, (storage::v3::LabelId), (const)); + MOCK_METHOD(bool, IsPrimaryKey, (storage::v3::LabelId, storage::v3::PropertyId), (const)); }; -class MockedLogicalOperator : query::v2::plan::LogicalOperator { +class MockedLogicalOperator : public plan::LogicalOperator { public: - MOCK_CONST_METHOD1(MakeCursor, query::v2::plan::UniqueCursorPtr(utils::MemoryResource *)); - MOCK_CONST_METHOD1(OutputSymbols, std::vector(const expr::SymbolTable &)); - MOCK_CONST_METHOD1(ModifiedSymbols, std::vector(const expr::SymbolTable &)); - MOCK_CONST_METHOD0(HasSingleInput, bool()); - MOCK_CONST_METHOD0(input, std::shared_ptr()); - MOCK_METHOD1(set_input, void(std::shared_ptr)); - MOCK_CONST_METHOD1(Clone, std::unique_ptr(query::v2::AstStorage *storage)); + MOCK_METHOD(plan::UniqueCursorPtr, MakeCursor, (utils::MemoryResource *), (const)); + MOCK_METHOD(std::vector, ModifiedSymbols, (const expr::SymbolTable &), (const)); + MOCK_METHOD(bool, HasSingleInput, (), (const)); + MOCK_METHOD(std::shared_ptr, input, (), (const)); + MOCK_METHOD(void, set_input, (std::shared_ptr)); + MOCK_METHOD(std::unique_ptr, Clone, (AstStorage * storage), (const)); + MOCK_METHOD(bool, Accept, (plan::HierarchicalLogicalOperatorVisitor & visitor)); }; -class MockedCursor : memgraph::query::v2::plan::Cursor { +class MockedCursor : public plan::Cursor { public: - MOCK_METHOD2(Pull, bool(query::v2::Frame &, expr::ExecutionContext &)); - MOCK_METHOD2(PullMultiple, void(query::v2::MultiFrame &, expr::ExecutionContext &)); - MOCK_METHOD0(Reset, void()); - MOCK_METHOD0(Shutdown, void()); + MOCK_METHOD(bool, Pull, (Frame &, expr::ExecutionContext &)); + MOCK_METHOD(void, PullMultiple, (MultiFrame &, expr::ExecutionContext &)); + MOCK_METHOD(void, Reset, ()); + MOCK_METHOD(void, Shutdown, ()); }; -} // namespace memgraph +inline expr::ExecutionContext MakeContext(const expr::AstStorage &storage, const expr::SymbolTable &symbol_table, + RequestRouterInterface *router, IdAllocator *id_alloc) { + expr::ExecutionContext context; + context.symbol_table = symbol_table; + context.evaluation_context.properties = NamesToProperties(storage.properties_, router); + context.evaluation_context.labels = NamesToLabels(storage.labels_, router); + context.edge_ids_alloc = id_alloc; + context.request_router = router; + return context; +} + +inline MockedLogicalOperator &BaseToMock(plan::LogicalOperator *op) { + return *static_cast(op); +} + +inline MockedCursor &BaseToMock(plan::Cursor *cursor) { return *static_cast(cursor); } + +} // namespace memgraph::query::v2 diff --git a/tests/unit/query_v2_create_expand_multiframe.cpp b/tests/unit/query_v2_create_expand_multiframe.cpp index f6f6567db..5b4c1e2ec 100644 --- a/tests/unit/query_v2_create_expand_multiframe.cpp +++ b/tests/unit/query_v2_create_expand_multiframe.cpp @@ -9,6 +9,7 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. +#include #include "mock_helpers.hpp" #include "query/v2/bindings/frame.hpp" @@ -22,24 +23,7 @@ #include "utils/logging.hpp" #include "utils/memory.hpp" -using namespace memgraph::query::v2; -using namespace memgraph::query::v2::plan; -namespace memgraph { -class TestTemplate : public testing::Test { - protected: - void SetUp() override {} -}; - -ExecutionContext MakeContext(const AstStorage &storage, const SymbolTable &symbol_table, RequestRouterInterface *router, - IdAllocator *id_alloc) { - ExecutionContext context; - context.symbol_table = symbol_table; - context.evaluation_context.properties = NamesToProperties(storage.properties_, router); - context.evaluation_context.labels = NamesToLabels(storage.labels_, router); - context.edge_ids_alloc = id_alloc; - context.request_router = router; - return context; -} +namespace memgraph::query::v2 { MultiFrame CreateMultiFrame(const size_t max_pos, const Symbol &src, const Symbol &dst, MockedRequestRouter *router) { static constexpr size_t frame_size = 100; @@ -60,14 +44,15 @@ MultiFrame CreateMultiFrame(const size_t max_pos, const Symbol &src, const Symbo return multi_frame; } -TEST_F(TestTemplate, CreateExpand) { - MockedRequestRouter router; +TEST(CreateExpandTest, Cursor) { + using testing::_; + using testing::Return; AstStorage ast; SymbolTable symbol_table; - query::v2::plan::NodeCreationInfo node; - query::v2::plan::EdgeCreationInfo edge; + plan::NodeCreationInfo node; + plan::EdgeCreationInfo edge; edge.edge_type = msgs::EdgeTypeId::FromUint(1); edge.direction = EdgeAtom::Direction::IN; auto id_alloc = IdAllocator(0, 100); @@ -75,14 +60,20 @@ TEST_F(TestTemplate, CreateExpand) { const auto &src = symbol_table.CreateSymbol("n", true); node.symbol = symbol_table.CreateSymbol("u", true); - auto create_expand = query::v2::plan::CreateExpand(node, edge, nullptr, src, true); + auto once_cur = plan::MakeUniqueCursorPtr(utils::NewDeleteResource()); + EXPECT_CALL(BaseToMock(once_cur.get()), PullMultiple(_, _)).Times(1); + + std::shared_ptr once_op = std::make_shared(); + EXPECT_CALL(BaseToMock(once_op.get()), MakeCursor(_)).Times(1).WillOnce(Return(std::move(once_cur))); + + auto create_expand = plan::CreateExpand(node, edge, once_op, src, true); auto cursor = create_expand.MakeCursor(utils::NewDeleteResource()); - EXPECT_CALL(router, CreateExpand(testing::_)) - .Times(1) - .WillOnce(::testing::Return(std::vector{})); + MockedRequestRouter router; + EXPECT_CALL(router, CreateExpand(_)).Times(1).WillOnce(Return(std::vector{})); auto context = MakeContext(ast, symbol_table, &router, &id_alloc); auto multi_frame = CreateMultiFrame(context.symbol_table.max_position(), src, node.symbol, &router); cursor->PullMultiple(multi_frame, context); } -} // namespace memgraph + +} // namespace memgraph::query::v2 From a9eca651df72cd3041fae6e23bdf69e05356cd0c Mon Sep 17 00:00:00 2001 From: Kostas Kyrimis Date: Wed, 14 Dec 2022 18:26:40 +0200 Subject: [PATCH 4/9] Address GH comments and fix a bug in ValidFramesModifier postincrement --- src/query/v2/multiframe.hpp | 2 +- src/query/v2/plan/operator.cpp | 35 ++++++------------- tests/unit/mock_helpers.hpp | 6 ++-- .../query_v2_create_expand_multiframe.cpp | 16 +++++++-- 4 files changed, 28 insertions(+), 31 deletions(-) diff --git a/src/query/v2/multiframe.hpp b/src/query/v2/multiframe.hpp index e13eb07ac..7d9b73700 100644 --- a/src/query/v2/multiframe.hpp +++ b/src/query/v2/multiframe.hpp @@ -168,7 +168,7 @@ class ValidFramesModifier { Iterator &operator++() { do { ptr_++; - } while (*this != iterator_wrapper_->end() && ptr_->IsValid()); + } while (*this != iterator_wrapper_->end() && !ptr_->IsValid()); return *this; } diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 012981e44..805b3ec51 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -2468,9 +2468,9 @@ class DistributedCreateExpandCursor : public Cursor { std::vector ExpandCreationInfoToRequests(MultiFrame &multi_frame, ExecutionContext &context) const { std::vector edge_requests; - auto reader = multi_frame.GetValidFramesModifier(); + auto frames_modifier = multi_frame.GetValidFramesModifier(); - for (auto &frame : reader) { + for (auto &frame : frames_modifier) { const auto &edge_info = self_.edge_info_; msgs::NewExpand request{.id = {context.edge_ids_alloc->AllocateId()}}; ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, nullptr, @@ -2489,35 +2489,22 @@ class DistributedCreateExpandCursor : public Cursor { request.properties.emplace_back(property_id, storage::v3::TypedValueToValue(value)); } } - // src, dest + TypedValue &v1_value = frame[self_.input_symbol_]; const auto &v1 = v1_value.ValueVertex(); const auto &v2 = OtherVertex(frame); + msgs::Edge edge{.src = request.src_vertex, + .dst = request.dest_vertex, + .properties = request.properties, + .id = request.id, + .type = request.type}; + frame[self_.edge_info_.symbol] = TypedValue(accessors::EdgeAccessor(std::move(edge), context.request_router)); // Set src and dest vertices // TODO(jbajic) Currently we are only handling scenario where vertices // are matched - const auto set_vertex = [](const auto &vertex, auto &vertex_id) { - vertex_id.first = vertex.PrimaryLabel(); - vertex_id.second = vertex.GetVertex().id.second; - }; - - std::invoke([&]() { - switch (edge_info.direction) { - case EdgeAtom::Direction::IN: { - set_vertex(v2, request.src_vertex); - set_vertex(v1, request.dest_vertex); - break; - } - case EdgeAtom::Direction::OUT: { - set_vertex(v1, request.src_vertex); - set_vertex(v2, request.dest_vertex); - break; - } - case EdgeAtom::Direction::BOTH: - LOG_FATAL("Must indicate exact expansion direction here"); - } - }); + request.src_vertex = v1.Id(); + request.dest_vertex = v2.Id(); edge_requests.push_back(std::move(request)); } diff --git a/tests/unit/mock_helpers.hpp b/tests/unit/mock_helpers.hpp index 0010d5986..6c03889ba 100644 --- a/tests/unit/mock_helpers.hpp +++ b/tests/unit/mock_helpers.hpp @@ -73,10 +73,10 @@ inline expr::ExecutionContext MakeContext(const expr::AstStorage &storage, const return context; } -inline MockedLogicalOperator &BaseToMock(plan::LogicalOperator *op) { - return *static_cast(op); +inline MockedLogicalOperator &BaseToMock(plan::LogicalOperator &op) { + return dynamic_cast(op); } -inline MockedCursor &BaseToMock(plan::Cursor *cursor) { return *static_cast(cursor); } +inline MockedCursor &BaseToMock(plan::Cursor &cursor) { return dynamic_cast(cursor); } } // namespace memgraph::query::v2 diff --git a/tests/unit/query_v2_create_expand_multiframe.cpp b/tests/unit/query_v2_create_expand_multiframe.cpp index 5b4c1e2ec..05b631dae 100644 --- a/tests/unit/query_v2_create_expand_multiframe.cpp +++ b/tests/unit/query_v2_create_expand_multiframe.cpp @@ -55,25 +55,35 @@ TEST(CreateExpandTest, Cursor) { plan::EdgeCreationInfo edge; edge.edge_type = msgs::EdgeTypeId::FromUint(1); edge.direction = EdgeAtom::Direction::IN; + edge.symbol = symbol_table.CreateSymbol("e", true); auto id_alloc = IdAllocator(0, 100); const auto &src = symbol_table.CreateSymbol("n", true); node.symbol = symbol_table.CreateSymbol("u", true); auto once_cur = plan::MakeUniqueCursorPtr(utils::NewDeleteResource()); - EXPECT_CALL(BaseToMock(once_cur.get()), PullMultiple(_, _)).Times(1); + EXPECT_CALL(BaseToMock(*once_cur), PullMultiple(_, _)).Times(1); std::shared_ptr once_op = std::make_shared(); - EXPECT_CALL(BaseToMock(once_op.get()), MakeCursor(_)).Times(1).WillOnce(Return(std::move(once_cur))); + EXPECT_CALL(BaseToMock(*once_op), MakeCursor(_)).Times(1).WillOnce(Return(std::move(once_cur))); auto create_expand = plan::CreateExpand(node, edge, once_op, src, true); auto cursor = create_expand.MakeCursor(utils::NewDeleteResource()); MockedRequestRouter router; - EXPECT_CALL(router, CreateExpand(_)).Times(1).WillOnce(Return(std::vector{})); + EXPECT_CALL(router, CreateExpand(_)) + .Times(1) + .WillOnce(Return(std::vector{msgs::CreateExpandResponse{}})); auto context = MakeContext(ast, symbol_table, &router, &id_alloc); auto multi_frame = CreateMultiFrame(context.symbol_table.max_position(), src, node.symbol, &router); cursor->PullMultiple(multi_frame, context); + + auto frames = multi_frame.GetValidFramesReader(); + for (auto &frame : frames) { + EXPECT_EQ(frame[edge.symbol].IsEdge(), true); + const auto &e = frame[edge.symbol].ValueEdge(); + EXPECT_EQ(e.EdgeType(), edge.edge_type); + } } } // namespace memgraph::query::v2 From 7e217e94b3bde975a8ce1c9cde930ffd249e64c7 Mon Sep 17 00:00:00 2001 From: jeremy Date: Mon, 19 Dec 2022 15:44:01 +0100 Subject: [PATCH 5/9] Tests: CreateMultiFrame create invalid frames Test uses real "once" instead of mocked version --- .../query_v2_create_expand_multiframe.cpp | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/tests/unit/query_v2_create_expand_multiframe.cpp b/tests/unit/query_v2_create_expand_multiframe.cpp index 05b631dae..8720656fd 100644 --- a/tests/unit/query_v2_create_expand_multiframe.cpp +++ b/tests/unit/query_v2_create_expand_multiframe.cpp @@ -26,12 +26,11 @@ namespace memgraph::query::v2 { MultiFrame CreateMultiFrame(const size_t max_pos, const Symbol &src, const Symbol &dst, MockedRequestRouter *router) { - static constexpr size_t frame_size = 100; - MultiFrame multi_frame(max_pos, frame_size, utils::NewDeleteResource()); + static constexpr size_t number_of_frames = 100; + MultiFrame multi_frame(max_pos, number_of_frames, utils::NewDeleteResource()); auto frames_populator = multi_frame.GetInvalidFramesPopulator(); size_t i = 0; for (auto &frame : frames_populator) { - frame.MakeValid(); auto &src_acc = frame.at(src); auto &dst_acc = frame.at(dst); auto v1 = msgs::Vertex{.id = {{msgs::LabelId::FromUint(1)}, {msgs::Value(static_cast(i++))}}}; @@ -41,6 +40,8 @@ MultiFrame CreateMultiFrame(const size_t max_pos, const Symbol &src, const Symbo dst_acc = TypedValue(query::v2::accessors::VertexAccessor(v2, mp, router)); } + multi_frame.MakeAllFramesInvalid(); + return multi_frame; } @@ -61,11 +62,8 @@ TEST(CreateExpandTest, Cursor) { const auto &src = symbol_table.CreateSymbol("n", true); node.symbol = symbol_table.CreateSymbol("u", true); - auto once_cur = plan::MakeUniqueCursorPtr(utils::NewDeleteResource()); - EXPECT_CALL(BaseToMock(*once_cur), PullMultiple(_, _)).Times(1); - - std::shared_ptr once_op = std::make_shared(); - EXPECT_CALL(BaseToMock(*once_op), MakeCursor(_)).Times(1).WillOnce(Return(std::move(once_cur))); + auto once_op = std::make_shared(); + auto once_cur = once_op->MakeCursor(utils::NewDeleteResource()); auto create_expand = plan::CreateExpand(node, edge, once_op, src, true); auto cursor = create_expand.MakeCursor(utils::NewDeleteResource()); @@ -79,11 +77,18 @@ TEST(CreateExpandTest, Cursor) { cursor->PullMultiple(multi_frame, context); auto frames = multi_frame.GetValidFramesReader(); + auto number_of_valid_frames = 0; for (auto &frame : frames) { + ++number_of_valid_frames; EXPECT_EQ(frame[edge.symbol].IsEdge(), true); const auto &e = frame[edge.symbol].ValueEdge(); EXPECT_EQ(e.EdgeType(), edge.edge_type); } + EXPECT_EQ(number_of_valid_frames, 1); + + auto invalid_frames = multi_frame.GetInvalidFramesPopulator(); + auto number_of_invalid_frames = std::distance(invalid_frames.begin(), invalid_frames.end()); + EXPECT_EQ(number_of_invalid_frames, 99); } } // namespace memgraph::query::v2 From 599b133a55ba56bd9725d9462b428758bbb582f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 12 Jan 2023 09:04:18 +0100 Subject: [PATCH 6/9] Fix edge direction when creating edges --- src/query/v2/plan/operator.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 805b3ec51..8681c6a2a 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -2503,8 +2503,20 @@ class DistributedCreateExpandCursor : public Cursor { // Set src and dest vertices // TODO(jbajic) Currently we are only handling scenario where vertices // are matched - request.src_vertex = v1.Id(); - request.dest_vertex = v2.Id(); + switch (edge_info.direction) { + case EdgeAtom::Direction::IN: { + request.src_vertex = v2.Id(); + request.dest_vertex = v1.Id(); + break; + } + case EdgeAtom::Direction::OUT: { + request.src_vertex = v1.Id(); + request.dest_vertex = v2.Id(); + break; + } + case EdgeAtom::Direction::BOTH: + LOG_FATAL("Must indicate exact expansion direction here"); + } edge_requests.push_back(std::move(request)); } From d7bd2cc7549b428ab7b850115d6d1ed3301cc219 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 12 Jan 2023 09:05:29 +0100 Subject: [PATCH 7/9] Eliminate copying expands --- src/query/v2/request_router.hpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/query/v2/request_router.hpp b/src/query/v2/request_router.hpp index 3dd2f164b..96d51b05f 100644 --- a/src/query/v2/request_router.hpp +++ b/src/query/v2/request_router.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -299,7 +299,8 @@ class RequestRouter : public RequestRouterInterface { MG_ASSERT(!new_edges.empty()); // create requests - std::vector> requests_to_be_sent = RequestsForCreateExpand(new_edges); + std::vector> requests_to_be_sent = + RequestsForCreateExpand(std::move(new_edges)); // begin all requests in parallel RunningRequests running_requests = {}; @@ -430,7 +431,7 @@ class RequestRouter : public RequestRouterInterface { } std::vector> RequestsForCreateExpand( - const std::vector &new_expands) { + std::vector new_expands) { std::map per_shard_request_table; auto ensure_shard_exists_in_table = [&per_shard_request_table, transaction_id = transaction_id_](const ShardMetadata &shard) { From b38a9b9c901c24c3bb99d62465c42282398aea45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 17 Jan 2023 20:25:10 +0100 Subject: [PATCH 8/9] Use `tests` namespace for tests --- tests/unit/mock_helpers.hpp | 6 +++--- tests/unit/query_v2_create_expand_multiframe.cpp | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/unit/mock_helpers.hpp b/tests/unit/mock_helpers.hpp index 6c03889ba..c7994586a 100644 --- a/tests/unit/mock_helpers.hpp +++ b/tests/unit/mock_helpers.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -18,7 +18,7 @@ #include "query/v2/plan/operator.hpp" #include "query/v2/request_router.hpp" -namespace memgraph::query::v2 { +namespace memgraph::query::v2::tests { class MockedRequestRouter : public RequestRouterInterface { public: MOCK_METHOD(std::vector, ScanVertices, (std::optional label)); @@ -79,4 +79,4 @@ inline MockedLogicalOperator &BaseToMock(plan::LogicalOperator &op) { inline MockedCursor &BaseToMock(plan::Cursor &cursor) { return dynamic_cast(cursor); } -} // namespace memgraph::query::v2 +} // namespace memgraph::query::v2::tests diff --git a/tests/unit/query_v2_create_expand_multiframe.cpp b/tests/unit/query_v2_create_expand_multiframe.cpp index 8720656fd..ebdc4a9a7 100644 --- a/tests/unit/query_v2_create_expand_multiframe.cpp +++ b/tests/unit/query_v2_create_expand_multiframe.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -23,7 +23,7 @@ #include "utils/logging.hpp" #include "utils/memory.hpp" -namespace memgraph::query::v2 { +namespace memgraph::query::v2::tests { MultiFrame CreateMultiFrame(const size_t max_pos, const Symbol &src, const Symbol &dst, MockedRequestRouter *router) { static constexpr size_t number_of_frames = 100; @@ -91,4 +91,4 @@ TEST(CreateExpandTest, Cursor) { EXPECT_EQ(number_of_invalid_frames, 99); } -} // namespace memgraph::query::v2 +} // namespace memgraph::query::v2::tests From a3b1676c42bb8a6a69d872ace5f0b8d75891ea63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 17 Jan 2023 20:25:28 +0100 Subject: [PATCH 9/9] Separate include blocks --- tests/unit/mock_helpers.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/mock_helpers.hpp b/tests/unit/mock_helpers.hpp index c7994586a..c522b8602 100644 --- a/tests/unit/mock_helpers.hpp +++ b/tests/unit/mock_helpers.hpp @@ -13,6 +13,7 @@ #include #include + #include "query/v2/common.hpp" #include "query/v2/context.hpp" #include "query/v2/plan/operator.hpp"