From ace1eb401f4aab75a5b955849957dcb96f149897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Sun, 15 Jan 2023 18:25:32 +0100 Subject: [PATCH 1/8] Make unit tests compile with new gtest version --- tests/unit/query_v2_create_node_multiframe.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/unit/query_v2_create_node_multiframe.cpp b/tests/unit/query_v2_create_node_multiframe.cpp index f19082783..39ce17b43 100644 --- a/tests/unit/query_v2_create_node_multiframe.cpp +++ b/tests/unit/query_v2_create_node_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 @@ -55,11 +55,7 @@ TEST(CreateNodeTest, CreateNodeCursor) { p.push_back(std::make_pair(msgs::PropertyId::FromUint(2), &literal)); node.properties.emplace<0>(std::move(p)); - 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 once_op = std::make_shared(); auto create_expand = plan::CreateNode(once_op, node); auto cursor = create_expand.MakeCursor(utils::NewDeleteResource()); From b2b9b1d5cbc06a110f34bbb250591ecb5d5d22e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Sun, 15 Jan 2023 18:25:48 +0100 Subject: [PATCH 2/8] Eliminate warnings about deprecated methods --- tests/unit/bfs_single_node.cpp | 32 +++++++++---------- tests/unit/cypher_main_visitor.cpp | 6 ++-- ..._print_ast_to_original_expression_test.cpp | 4 +-- tests/unit/query_plan.cpp | 4 +-- .../query_v2_create_expand_multiframe.cpp | 3 +- tests/unit/query_v2_cypher_main_visitor.cpp | 6 ++-- tests/unit/storage_v3.cpp | 6 ++-- tests/unit/storage_v3_edge.cpp | 6 ++-- tests/unit/storage_v3_isolation_level.cpp | 6 ++-- tests/unit/utils_csv_parsing.cpp | 4 +-- tests/unit/utils_file_locker.cpp | 8 ++--- tests/unit/utils_memory.cpp | 4 +-- 12 files changed, 44 insertions(+), 45 deletions(-) diff --git a/tests/unit/bfs_single_node.cpp b/tests/unit/bfs_single_node.cpp index 93002eef5..eab4f3973 100644 --- a/tests/unit/bfs_single_node.cpp +++ b/tests/unit/bfs_single_node.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 @@ -88,14 +88,14 @@ TEST_P(SingleNodeBfsTest, All) { std::unique_ptr SingleNodeBfsTest::db_{nullptr}; -INSTANTIATE_TEST_CASE_P(DirectionAndExpansionDepth, SingleNodeBfsTest, - testing::Combine(testing::Range(-1, kVertexCount), testing::Range(-1, kVertexCount), - testing::Values(EdgeAtom::Direction::OUT, EdgeAtom::Direction::IN, - EdgeAtom::Direction::BOTH), - testing::Values(std::vector{}), testing::Bool(), - testing::Values(FilterLambdaType::NONE))); +INSTANTIATE_TEST_SUITE_P(DirectionAndExpansionDepth, SingleNodeBfsTest, + testing::Combine(testing::Range(-1, kVertexCount), testing::Range(-1, kVertexCount), + testing::Values(EdgeAtom::Direction::OUT, EdgeAtom::Direction::IN, + EdgeAtom::Direction::BOTH), + testing::Values(std::vector{}), testing::Bool(), + testing::Values(FilterLambdaType::NONE))); -INSTANTIATE_TEST_CASE_P( +INSTANTIATE_TEST_SUITE_P( EdgeType, SingleNodeBfsTest, testing::Combine(testing::Values(-1), testing::Values(-1), testing::Values(EdgeAtom::Direction::OUT, EdgeAtom::Direction::IN, EdgeAtom::Direction::BOTH), @@ -103,11 +103,11 @@ INSTANTIATE_TEST_CASE_P( std::vector{"b"}, std::vector{"a", "b"}), testing::Bool(), testing::Values(FilterLambdaType::NONE))); -INSTANTIATE_TEST_CASE_P(FilterLambda, SingleNodeBfsTest, - testing::Combine(testing::Values(-1), testing::Values(-1), - testing::Values(EdgeAtom::Direction::OUT, EdgeAtom::Direction::IN, - EdgeAtom::Direction::BOTH), - testing::Values(std::vector{}), testing::Bool(), - testing::Values(FilterLambdaType::NONE, FilterLambdaType::USE_FRAME, - FilterLambdaType::USE_FRAME_NULL, FilterLambdaType::USE_CTX, - FilterLambdaType::ERROR))); +INSTANTIATE_TEST_SUITE_P(FilterLambda, SingleNodeBfsTest, + testing::Combine(testing::Values(-1), testing::Values(-1), + testing::Values(EdgeAtom::Direction::OUT, EdgeAtom::Direction::IN, + EdgeAtom::Direction::BOTH), + testing::Values(std::vector{}), testing::Bool(), + testing::Values(FilterLambdaType::NONE, FilterLambdaType::USE_FRAME, + FilterLambdaType::USE_FRAME_NULL, FilterLambdaType::USE_CTX, + FilterLambdaType::ERROR))); diff --git a/tests/unit/cypher_main_visitor.cpp b/tests/unit/cypher_main_visitor.cpp index 40aeb0161..93f0653b4 100644 --- a/tests/unit/cypher_main_visitor.cpp +++ b/tests/unit/cypher_main_visitor.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 @@ -294,7 +294,7 @@ std::shared_ptr gAstGeneratorTypes[] = { std::make_shared(), }; -INSTANTIATE_TEST_CASE_P(AstGeneratorTypes, CypherMainVisitorTest, ::testing::ValuesIn(gAstGeneratorTypes)); +INSTANTIATE_TEST_SUITE_P(AstGeneratorTypes, CypherMainVisitorTest, ::testing::ValuesIn(gAstGeneratorTypes)); // NOTE: The above used to use *Typed Tests* functionality of gtest library. // Unfortunately, the compilation time of this test increased to full 2 minutes! @@ -308,7 +308,7 @@ INSTANTIATE_TEST_CASE_P(AstGeneratorTypes, CypherMainVisitorTest, ::testing::Val // ClonedAstGenerator, CachedAstGenerator> // AstGeneratorTypes; // -// TYPED_TEST_CASE(CypherMainVisitorTest, AstGeneratorTypes); +// TYPED_TEST_SUITE(CypherMainVisitorTest, AstGeneratorTypes); TEST_P(CypherMainVisitorTest, SyntaxException) { auto &ast_generator = *GetParam(); diff --git a/tests/unit/pretty_print_ast_to_original_expression_test.cpp b/tests/unit/pretty_print_ast_to_original_expression_test.cpp index e5d77ae0e..a2144c8aa 100644 --- a/tests/unit/pretty_print_ast_to_original_expression_test.cpp +++ b/tests/unit/pretty_print_ast_to_original_expression_test.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 @@ -67,7 +67,7 @@ TEST_P(ExpressiontoStringTest, Example) { EXPECT_EQ(rewritten_expression, rewritten_expression2); } -INSTANTIATE_TEST_CASE_P( +INSTANTIATE_TEST_SUITE_P( PARAMETER, ExpressiontoStringTest, ::testing::Values( std::make_pair(std::string("2 / 1"), std::string("(2 / 1)")), diff --git a/tests/unit/query_plan.cpp b/tests/unit/query_plan.cpp index 93d2f33c7..935709cec 100644 --- a/tests/unit/query_plan.cpp +++ b/tests/unit/query_plan.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 @@ -90,7 +90,7 @@ void DeleteListContent(std::list *list) { delete ptr; } } -TYPED_TEST_CASE(TestPlanner, PlannerTypes); +TYPED_TEST_SUITE(TestPlanner, PlannerTypes); TYPED_TEST(TestPlanner, MatchNodeReturn) { // Test MATCH (n) RETURN n diff --git a/tests/unit/query_v2_create_expand_multiframe.cpp b/tests/unit/query_v2_create_expand_multiframe.cpp index 8720656fd..e978a6bb5 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 @@ -63,7 +63,6 @@ TEST(CreateExpandTest, Cursor) { node.symbol = symbol_table.CreateSymbol("u", true); 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()); diff --git a/tests/unit/query_v2_cypher_main_visitor.cpp b/tests/unit/query_v2_cypher_main_visitor.cpp index d7e4169e2..4d5311a6e 100644 --- a/tests/unit/query_v2_cypher_main_visitor.cpp +++ b/tests/unit/query_v2_cypher_main_visitor.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 @@ -299,7 +299,7 @@ std::shared_ptr gAstGeneratorTypes[] = { std::make_shared(), }; -INSTANTIATE_TEST_CASE_P(AstGeneratorTypes, CypherMainVisitorTest, ::testing::ValuesIn(gAstGeneratorTypes)); +INSTANTIATE_TEST_SUITE_P(AstGeneratorTypes, CypherMainVisitorTest, ::testing::ValuesIn(gAstGeneratorTypes)); // NOTE: The above used to use *Typed Tests* functionality of gtest library. // Unfortunately, the compilation time of this test increased to full 2 minutes! @@ -313,7 +313,7 @@ INSTANTIATE_TEST_CASE_P(AstGeneratorTypes, CypherMainVisitorTest, ::testing::Val // ClonedAstGenerator, CachedAstGenerator> // AstGeneratorTypes; // -// TYPED_TEST_CASE(CypherMainVisitorTest, AstGeneratorTypes); +// TYPED_TEST_SUITE(CypherMainVisitorTest, AstGeneratorTypes); TEST_P(CypherMainVisitorTest, SyntaxException) { auto &ast_generator = *GetParam(); diff --git a/tests/unit/storage_v3.cpp b/tests/unit/storage_v3.cpp index 6f6902d1c..1832165f9 100644 --- a/tests/unit/storage_v3.cpp +++ b/tests/unit/storage_v3.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 @@ -82,8 +82,8 @@ class StorageV3 : public ::testing::TestWithParam { Config{.gc = {.reclamation_interval = reclamation_interval}}}; coordinator::Hlc last_hlc{0, io::Time{}}; }; -INSTANTIATE_TEST_CASE_P(WithGc, StorageV3, ::testing::Values(true)); -INSTANTIATE_TEST_CASE_P(WithoutGc, StorageV3, ::testing::Values(false)); +INSTANTIATE_TEST_SUITE_P(WithGc, StorageV3, ::testing::Values(true)); +INSTANTIATE_TEST_SUITE_P(WithoutGc, StorageV3, ::testing::Values(false)); // NOLINTNEXTLINE(hicpp-special-member-functions) TEST_P(StorageV3, Commit) { diff --git a/tests/unit/storage_v3_edge.cpp b/tests/unit/storage_v3_edge.cpp index 3d2ab8bbd..8637db11c 100644 --- a/tests/unit/storage_v3_edge.cpp +++ b/tests/unit/storage_v3_edge.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 @@ -60,8 +60,8 @@ class StorageEdgeTest : public ::testing::TestWithParam { coordinator::Hlc last_hlc{0, io::Time{}}; }; -INSTANTIATE_TEST_CASE_P(EdgesWithProperties, StorageEdgeTest, ::testing::Values(true)); -INSTANTIATE_TEST_CASE_P(EdgesWithoutProperties, StorageEdgeTest, ::testing::Values(false)); +INSTANTIATE_TEST_SUITE_P(EdgesWithProperties, StorageEdgeTest, ::testing::Values(true)); +INSTANTIATE_TEST_SUITE_P(EdgesWithoutProperties, StorageEdgeTest, ::testing::Values(false)); // NOLINTNEXTLINE(hicpp-special-member-functions) TEST_P(StorageEdgeTest, EdgeCreateFromSmallerCommit) { diff --git a/tests/unit/storage_v3_isolation_level.cpp b/tests/unit/storage_v3_isolation_level.cpp index 6b661f632..bd0f27b6b 100644 --- a/tests/unit/storage_v3_isolation_level.cpp +++ b/tests/unit/storage_v3_isolation_level.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 @@ -135,6 +135,6 @@ TEST_P(StorageIsolationLevelTest, Visibility) { } } -INSTANTIATE_TEST_CASE_P(ParameterizedStorageIsolationLevelTests, StorageIsolationLevelTest, - ::testing::ValuesIn(isolation_levels), StorageIsolationLevelTest::PrintToStringParamName()); +INSTANTIATE_TEST_SUITE_P(ParameterizedStorageIsolationLevelTests, StorageIsolationLevelTest, + ::testing::ValuesIn(isolation_levels), StorageIsolationLevelTest::PrintToStringParamName()); } // namespace memgraph::storage::v3::tests diff --git a/tests/unit/utils_csv_parsing.cpp b/tests/unit/utils_csv_parsing.cpp index 3c852b171..e8d6c2241 100644 --- a/tests/unit/utils_csv_parsing.cpp +++ b/tests/unit/utils_csv_parsing.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 @@ -330,4 +330,4 @@ TEST_P(CsvReaderTest, EmptyColumns) { } } -INSTANTIATE_TEST_CASE_P(NewlineParameterizedTest, CsvReaderTest, ::testing::Values("\n", "\r\n")); +INSTANTIATE_TEST_SUITE_P(NewlineParameterizedTest, CsvReaderTest, ::testing::Values("\n", "\r\n")); diff --git a/tests/unit/utils_file_locker.cpp b/tests/unit/utils_file_locker.cpp index 21c71b1a3..f2217953e 100644 --- a/tests/unit/utils_file_locker.cpp +++ b/tests/unit/utils_file_locker.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 @@ -190,9 +190,9 @@ TEST_P(FileLockerParameterizedTest, RemovePath) { std::filesystem::current_path(save_path); } -INSTANTIATE_TEST_CASE_P(FileLockerPathVariantTests, FileLockerParameterizedTest, - ::testing::Values(std::make_tuple(false, false), std::make_tuple(false, true), - std::make_tuple(true, false), std::make_tuple(true, true))); +INSTANTIATE_TEST_SUITE_P(FileLockerPathVariantTests, FileLockerParameterizedTest, + ::testing::Values(std::make_tuple(false, false), std::make_tuple(false, true), + std::make_tuple(true, false), std::make_tuple(true, true))); TEST_F(FileLockerTest, MultipleLockers) { CreateFiles(3); diff --git a/tests/unit/utils_memory.cpp b/tests/unit/utils_memory.cpp index 70bf85653..73f78d545 100644 --- a/tests/unit/utils_memory.cpp +++ b/tests/unit/utils_memory.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 @@ -393,7 +393,7 @@ class AllocatorTest : public ::testing::Test {}; using ContainersWithAllocators = ::testing::Types; -TYPED_TEST_CASE(AllocatorTest, ContainersWithAllocators); +TYPED_TEST_SUITE(AllocatorTest, ContainersWithAllocators); TYPED_TEST(AllocatorTest, PropagatesToStdUsesAllocator) { std::vector> vec(memgraph::utils::NewDeleteResource()); From b30137ab7ac7bdd5005b6f5ffc23d45131e6fa6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Sun, 15 Jan 2023 18:39:58 +0100 Subject: [PATCH 3/8] Improve unit tests to catch bug --- src/query/v2/requests.hpp | 3 ++- tests/unit/query_v2_create_node_multiframe.cpp | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/query/v2/requests.hpp b/src/query/v2/requests.hpp index 9ff9a1bae..2335fea7d 100644 --- a/src/query/v2/requests.hpp +++ b/src/query/v2/requests.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 @@ -36,6 +36,7 @@ struct Value; struct Label { LabelId id; friend bool operator==(const Label &lhs, const Label &rhs) { return lhs.id == rhs.id; } + friend bool operator==(const Label &lhs, const LabelId &rhs) { return lhs.id == rhs; } }; // TODO(kostasrim) update this with CompoundKey, same for the rest of the file. diff --git a/tests/unit/query_v2_create_node_multiframe.cpp b/tests/unit/query_v2_create_node_multiframe.cpp index 39ce17b43..5e5d83249 100644 --- a/tests/unit/query_v2_create_node_multiframe.cpp +++ b/tests/unit/query_v2_create_node_multiframe.cpp @@ -36,15 +36,13 @@ MultiFrame CreateMultiFrame(const size_t max_pos) { TEST(CreateNodeTest, CreateNodeCursor) { using testing::_; + using testing::ElementsAre; using testing::Return; AstStorage ast; SymbolTable symbol_table; plan::NodeCreationInfo node; - plan::EdgeCreationInfo edge; - edge.edge_type = msgs::EdgeTypeId::FromUint(1); - edge.direction = EdgeAtom::Direction::IN; auto id_alloc = IdAllocator(0, 100); node.symbol = symbol_table.CreateSymbol("n", true); @@ -67,5 +65,19 @@ TEST(CreateNodeTest, CreateNodeCursor) { auto context = MakeContext(ast, symbol_table, &router, &id_alloc); auto multi_frame = CreateMultiFrame(context.symbol_table.max_position()); 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[node.symbol].IsEdge(), true); + const auto &n = frame[node.symbol].ValueVertex(); + EXPECT_THAT(n.Labels(), ElementsAre(msgs::Label{msgs::LabelId::FromUint(2)})); + } + 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 c139856b2a9b17ea52bc9c38107eeaa7f3b966f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Sun, 15 Jan 2023 18:52:36 +0100 Subject: [PATCH 4/8] Fix unit tests --- src/query/v2/plan/operator.cpp | 14 +++++++------- tests/unit/query_v2_create_node_multiframe.cpp | 18 +++++++++--------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index da282387c..630599276 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 @@ -192,7 +192,7 @@ class DistributedCreateNodeCursor : public Cursor { void PullMultiple(MultiFrame &multi_frame, ExecutionContext &context) override { SCOPED_PROFILE_OP("CreateNodeMF"); input_cursor_->PullMultiple(multi_frame, context); - auto &request_router = context.request_router; + auto *request_router = context.request_router; { SCOPED_REQUEST_WAIT_PROFILE; request_router->CreateVertices(NodeCreationInfoToRequests(context, multi_frame)); @@ -259,10 +259,10 @@ class DistributedCreateNodeCursor : public Cursor { } void PlaceNodesOnTheMultiFrame(MultiFrame &multi_frame, ExecutionContext &context) { - auto multi_frame_reader = multi_frame.GetValidFramesConsumer(); + auto multi_frame_modifier = multi_frame.GetValidFramesModifier(); size_t i = 0; - MG_ASSERT(std::distance(multi_frame_reader.begin(), multi_frame_reader.end())); - for (auto &frame : multi_frame_reader) { + MG_ASSERT(std::distance(multi_frame_modifier.begin(), multi_frame_modifier.end())); + for (auto &frame : multi_frame_modifier) { const auto primary_label = msgs::Label{.id = nodes_info_[0]->labels[0]}; msgs::Vertex v{.id = std::make_pair(primary_label, primary_keys_[i])}; frame[nodes_info_.front()->symbol] = TypedValue( @@ -272,8 +272,8 @@ class DistributedCreateNodeCursor : public Cursor { std::vector NodeCreationInfoToRequests(ExecutionContext &context, MultiFrame &multi_frame) { std::vector requests; - auto multi_frame_reader = multi_frame.GetValidFramesConsumer(); - for (auto &frame : multi_frame_reader) { + auto multi_frame_modifier = multi_frame.GetValidFramesModifier(); + for (auto &frame : multi_frame_modifier) { msgs::PrimaryKey pk; for (const auto &node_info : nodes_info_) { msgs::NewVertex rqst; diff --git a/tests/unit/query_v2_create_node_multiframe.cpp b/tests/unit/query_v2_create_node_multiframe.cpp index 5e5d83249..b04b00e96 100644 --- a/tests/unit/query_v2_create_node_multiframe.cpp +++ b/tests/unit/query_v2_create_node_multiframe.cpp @@ -9,6 +9,7 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. +#include "gmock/gmock.h" #include "mock_helpers.hpp" #include "query/v2/bindings/frame.hpp" @@ -26,17 +27,13 @@ namespace memgraph::query::v2 { MultiFrame CreateMultiFrame(const size_t max_pos) { static constexpr size_t frame_size = 100; MultiFrame multi_frame(max_pos, frame_size, utils::NewDeleteResource()); - auto frames_populator = multi_frame.GetInvalidFramesPopulator(); - for (auto &frame : frames_populator) { - frame.MakeValid(); - } return multi_frame; } TEST(CreateNodeTest, CreateNodeCursor) { using testing::_; - using testing::ElementsAre; + using testing::IsEmpty; using testing::Return; AstStorage ast; @@ -46,11 +43,12 @@ TEST(CreateNodeTest, CreateNodeCursor) { auto id_alloc = IdAllocator(0, 100); node.symbol = symbol_table.CreateSymbol("n", true); - node.labels.push_back(msgs::LabelId::FromUint(2)); + const auto primary_label_id = msgs::LabelId::FromUint(2); + node.labels.push_back(primary_label_id); auto literal = PrimitiveLiteral(); literal.value_ = TypedValue(static_cast(200)); auto p = plan::PropertiesMapList{}; - p.push_back(std::make_pair(msgs::PropertyId::FromUint(2), &literal)); + p.push_back(std::make_pair(msgs::PropertyId::FromUint(3), &literal)); node.properties.emplace<0>(std::move(p)); auto once_op = std::make_shared(); @@ -70,9 +68,11 @@ TEST(CreateNodeTest, CreateNodeCursor) { auto number_of_valid_frames = 0; for (auto &frame : frames) { ++number_of_valid_frames; - EXPECT_EQ(frame[node.symbol].IsEdge(), true); + EXPECT_EQ(frame[node.symbol].IsVertex(), true); const auto &n = frame[node.symbol].ValueVertex(); - EXPECT_THAT(n.Labels(), ElementsAre(msgs::Label{msgs::LabelId::FromUint(2)})); + EXPECT_THAT(n.Labels(), IsEmpty()); + EXPECT_EQ(n.PrimaryLabel(), primary_label_id); + // TODO(antaljanosbenjamin): Check primary key } EXPECT_EQ(number_of_valid_frames, 1); From e40f7f507b422ddb0daa4379bd529fce2234cfa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 16 Jan 2023 08:40:43 +0100 Subject: [PATCH 5/8] Fix pull logic for multiframe --- src/query/v2/interpreter.cpp | 4 ++-- src/query/v2/multiframe.cpp | 6 +++++- src/query/v2/multiframe.hpp | 3 ++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/query/v2/interpreter.cpp b/src/query/v2/interpreter.cpp index fde11ac00..594942aec 100644 --- a/src/query/v2/interpreter.cpp +++ b/src/query/v2/interpreter.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 @@ -734,7 +734,7 @@ std::optional PullPlan::PullMultiple(AnyStrea // Returns true if a result was pulled. const auto pull_result = [&]() -> bool { cursor_->PullMultiple(multi_frame_, ctx_); - return multi_frame_.HasValidFrame(); + return !multi_frame_.HasInvalidFrame(); }; const auto stream_values = [&output_symbols, &stream](const Frame &frame) { diff --git a/src/query/v2/multiframe.cpp b/src/query/v2/multiframe.cpp index 2cb591153..0ddfd3aa7 100644 --- a/src/query/v2/multiframe.cpp +++ b/src/query/v2/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 @@ -48,6 +48,10 @@ bool MultiFrame::HasValidFrame() const noexcept { return std::any_of(frames_.begin(), frames_.end(), [](auto &frame) { return frame.IsValid(); }); } +bool MultiFrame::HasInvalidFrame() const noexcept { + return std::any_of(frames_.rbegin(), frames_.rend(), [](auto &frame) { return !frame.IsValid(); }); +} + // NOLINTNEXTLINE (bugprone-exception-escape) void MultiFrame::DefragmentValidFrames() noexcept { /* diff --git a/src/query/v2/multiframe.hpp b/src/query/v2/multiframe.hpp index 7d9b73700..b46be171a 100644 --- a/src/query/v2/multiframe.hpp +++ b/src/query/v2/multiframe.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 @@ -81,6 +81,7 @@ class MultiFrame { void MakeAllFramesInvalid() noexcept; bool HasValidFrame() const noexcept; + bool HasInvalidFrame() const noexcept; inline utils::MemoryResource *GetMemoryResource() { return frames_[0].GetMemoryResource(); } From 392f6e2b730f644f7707a7861b81dcc43ee71280 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 16 Jan 2023 08:57:23 +0100 Subject: [PATCH 6/8] Reduce the number of node infos to a maximum of one --- src/query/v2/plan/operator.cpp | 141 +++++++++++++++------------------ 1 file changed, 65 insertions(+), 76 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 630599276..2f191c113 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -170,9 +170,8 @@ uint64_t ComputeProfilingKey(const T *obj) { class DistributedCreateNodeCursor : public Cursor { public: using InputOperator = std::shared_ptr; - DistributedCreateNodeCursor(const InputOperator &op, utils::MemoryResource *mem, - std::vector nodes_info) - : input_cursor_(op->MakeCursor(mem)), nodes_info_(std::move(nodes_info)) {} + DistributedCreateNodeCursor(const InputOperator &op, utils::MemoryResource *mem, const NodeCreationInfo &node_info) + : input_cursor_(op->MakeCursor(mem)), node_info_(node_info) {} bool Pull(Frame &frame, ExecutionContext &context) override { SCOPED_PROFILE_OP("CreateNode"); @@ -206,27 +205,78 @@ class DistributedCreateNodeCursor : public Cursor { void PlaceNodeOnTheFrame(Frame &frame, ExecutionContext &context) { // TODO(kostasrim) Make this work with batching - const auto primary_label = msgs::Label{.id = nodes_info_[0]->labels[0]}; + const auto primary_label = msgs::Label{.id = node_info_.labels[0]}; msgs::Vertex v{.id = std::make_pair(primary_label, primary_keys_[0])}; - frame[nodes_info_.front()->symbol] = + frame[node_info_.symbol] = TypedValue(query::v2::accessors::VertexAccessor(std::move(v), src_vertex_props_[0], context.request_router)); } std::vector NodeCreationInfoToRequest(ExecutionContext &context, Frame &frame) { std::vector requests; - // TODO(kostasrim) this assertion should be removed once we support multiple vertex creation - MG_ASSERT(nodes_info_.size() == 1); msgs::PrimaryKey pk; - for (const auto &node_info : nodes_info_) { + msgs::NewVertex rqst; + MG_ASSERT(!node_info_.labels.empty(), "Cannot determine primary label"); + const auto primary_label = node_info_.labels[0]; + // TODO(jbajic) Fix properties not send, + // suggestion: ignore distinction between properties and primary keys + // since schema validation is done on storage side + ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, nullptr, + storage::v3::View::NEW); + if (const auto *node_info_properties = std::get_if(&node_info_.properties)) { + for (const auto &[key, value_expression] : *node_info_properties) { + TypedValue val = value_expression->Accept(evaluator); + if (context.request_router->IsPrimaryKey(primary_label, key)) { + rqst.primary_key.push_back(TypedValueToValue(val)); + pk.push_back(TypedValueToValue(val)); + } + } + } else { + auto property_map = evaluator.Visit(*std::get(node_info_.properties)).ValueMap(); + for (const auto &[key, value] : property_map) { + auto key_str = std::string(key); + auto property_id = context.request_router->NameToProperty(key_str); + if (context.request_router->IsPrimaryKey(primary_label, property_id)) { + rqst.primary_key.push_back(TypedValueToValue(value)); + pk.push_back(TypedValueToValue(value)); + } + } + } + + // TODO(kostasrim) Copy non primary labels as well + rqst.label_ids.push_back(msgs::Label{.id = primary_label}); + src_vertex_props_.push_back(rqst.properties); + requests.push_back(std::move(rqst)); + + primary_keys_.push_back(std::move(pk)); + return requests; + } + + void PlaceNodesOnTheMultiFrame(MultiFrame &multi_frame, ExecutionContext &context) { + auto multi_frame_modifier = multi_frame.GetValidFramesModifier(); + size_t i = 0; + MG_ASSERT(std::distance(multi_frame_modifier.begin(), multi_frame_modifier.end())); + for (auto &frame : multi_frame_modifier) { + const auto primary_label = msgs::Label{.id = node_info_.labels[0]}; + msgs::Vertex v{.id = std::make_pair(primary_label, primary_keys_[i])}; + frame[node_info_.symbol] = TypedValue( + query::v2::accessors::VertexAccessor(std::move(v), src_vertex_props_[i++], context.request_router)); + } + } + + std::vector NodeCreationInfoToRequests(ExecutionContext &context, MultiFrame &multi_frame) { + std::vector requests; + auto multi_frame_modifier = multi_frame.GetValidFramesModifier(); + for (auto &frame : multi_frame_modifier) { + msgs::PrimaryKey pk; msgs::NewVertex rqst; - MG_ASSERT(!node_info->labels.empty(), "Cannot determine primary label"); - const auto primary_label = node_info->labels[0]; + MG_ASSERT(!node_info_.labels.empty(), "Cannot determine primary label"); + const auto primary_label = node_info_.labels[0]; // TODO(jbajic) Fix properties not send, // suggestion: ignore distinction between properties and primary keys // since schema validation is done on storage side ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, nullptr, storage::v3::View::NEW); - if (const auto *node_info_properties = std::get_if(&node_info->properties)) { + if (const auto *node_info_properties = std::get_if(&node_info_.properties)) { for (const auto &[key, value_expression] : *node_info_properties) { TypedValue val = value_expression->Accept(evaluator); if (context.request_router->IsPrimaryKey(primary_label, key)) { @@ -235,7 +285,7 @@ class DistributedCreateNodeCursor : public Cursor { } } } else { - auto property_map = evaluator.Visit(*std::get(node_info->properties)).ValueMap(); + auto property_map = evaluator.Visit(*std::get(node_info_.properties)).ValueMap(); for (const auto &[key, value] : property_map) { auto key_str = std::string(key); auto property_id = context.request_router->NameToProperty(key_str); @@ -246,80 +296,19 @@ class DistributedCreateNodeCursor : public Cursor { } } - if (node_info->labels.empty()) { - throw QueryRuntimeException("Primary label must be defined!"); - } // TODO(kostasrim) Copy non primary labels as well rqst.label_ids.push_back(msgs::Label{.id = primary_label}); src_vertex_props_.push_back(rqst.properties); requests.push_back(std::move(rqst)); - } - primary_keys_.push_back(std::move(pk)); - return requests; - } - - void PlaceNodesOnTheMultiFrame(MultiFrame &multi_frame, ExecutionContext &context) { - auto multi_frame_modifier = multi_frame.GetValidFramesModifier(); - size_t i = 0; - MG_ASSERT(std::distance(multi_frame_modifier.begin(), multi_frame_modifier.end())); - for (auto &frame : multi_frame_modifier) { - const auto primary_label = msgs::Label{.id = nodes_info_[0]->labels[0]}; - msgs::Vertex v{.id = std::make_pair(primary_label, primary_keys_[i])}; - frame[nodes_info_.front()->symbol] = TypedValue( - query::v2::accessors::VertexAccessor(std::move(v), src_vertex_props_[i++], context.request_router)); - } - } - - std::vector NodeCreationInfoToRequests(ExecutionContext &context, MultiFrame &multi_frame) { - std::vector requests; - auto multi_frame_modifier = multi_frame.GetValidFramesModifier(); - for (auto &frame : multi_frame_modifier) { - msgs::PrimaryKey pk; - for (const auto &node_info : nodes_info_) { - msgs::NewVertex rqst; - MG_ASSERT(!node_info->labels.empty(), "Cannot determine primary label"); - const auto primary_label = node_info->labels[0]; - // TODO(jbajic) Fix properties not send, - // suggestion: ignore distinction between properties and primary keys - // since schema validation is done on storage side - ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, nullptr, - storage::v3::View::NEW); - if (const auto *node_info_properties = std::get_if(&node_info->properties)) { - for (const auto &[key, value_expression] : *node_info_properties) { - TypedValue val = value_expression->Accept(evaluator); - if (context.request_router->IsPrimaryKey(primary_label, key)) { - rqst.primary_key.push_back(TypedValueToValue(val)); - pk.push_back(TypedValueToValue(val)); - } - } - } else { - auto property_map = evaluator.Visit(*std::get(node_info->properties)).ValueMap(); - for (const auto &[key, value] : property_map) { - auto key_str = std::string(key); - auto property_id = context.request_router->NameToProperty(key_str); - if (context.request_router->IsPrimaryKey(primary_label, property_id)) { - rqst.primary_key.push_back(TypedValueToValue(value)); - pk.push_back(TypedValueToValue(value)); - } - } - } - - if (node_info->labels.empty()) { - throw QueryRuntimeException("Primary label must be defined!"); - } - // TODO(kostasrim) Copy non primary labels as well - rqst.label_ids.push_back(msgs::Label{.id = primary_label}); - src_vertex_props_.push_back(rqst.properties); - requests.push_back(std::move(rqst)); - } primary_keys_.push_back(std::move(pk)); } + return requests; } private: const UniqueCursorPtr input_cursor_; - std::vector nodes_info_; + NodeCreationInfo node_info_; std::vector>> src_vertex_props_; std::vector primary_keys_; }; @@ -364,7 +353,7 @@ ACCEPT_WITH_INPUT(CreateNode) UniqueCursorPtr CreateNode::MakeCursor(utils::MemoryResource *mem) const { EventCounter::IncrementCounter(EventCounter::CreateNodeOperator); - return MakeUniqueCursorPtr(mem, input_, mem, std::vector{&this->node_info_}); + return MakeUniqueCursorPtr(mem, input_, mem, this->node_info_); } std::vector CreateNode::ModifiedSymbols(const SymbolTable &table) const { From 920ad277a5e44a34083f0030d3618695a7eb42bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 16 Jan 2023 09:03:35 +0100 Subject: [PATCH 7/8] Add assertion about primary label --- src/query/v2/plan/operator.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 2f191c113..4ea189772 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -271,6 +271,7 @@ class DistributedCreateNodeCursor : public Cursor { msgs::NewVertex rqst; MG_ASSERT(!node_info_.labels.empty(), "Cannot determine primary label"); const auto primary_label = node_info_.labels[0]; + MG_ASSERT(context.request_router->IsPrimaryLabel(primary_label), "First label has to be a primary label!"); // TODO(jbajic) Fix properties not send, // suggestion: ignore distinction between properties and primary keys // since schema validation is done on storage side From 775e950dbae0bc34339c91bd9def081c89f66308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 16 Jan 2023 10:16:12 +0100 Subject: [PATCH 8/8] Update unit tests to test the new logic --- tests/unit/query_v2_create_node_multiframe.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/unit/query_v2_create_node_multiframe.cpp b/tests/unit/query_v2_create_node_multiframe.cpp index b04b00e96..014fff6ce 100644 --- a/tests/unit/query_v2_create_node_multiframe.cpp +++ b/tests/unit/query_v2_create_node_multiframe.cpp @@ -56,10 +56,9 @@ TEST(CreateNodeTest, CreateNodeCursor) { auto create_expand = plan::CreateNode(once_op, node); auto cursor = create_expand.MakeCursor(utils::NewDeleteResource()); MockedRequestRouter router; - EXPECT_CALL(router, CreateVertices(testing::_)) - .Times(1) - .WillOnce(::testing::Return(std::vector{})); - EXPECT_CALL(router, IsPrimaryKey(testing::_, testing::_)).WillRepeatedly(::testing::Return(true)); + EXPECT_CALL(router, CreateVertices(_)).Times(1).WillOnce(Return(std::vector{})); + EXPECT_CALL(router, IsPrimaryLabel(_)).WillRepeatedly(Return(true)); + EXPECT_CALL(router, IsPrimaryKey(_, _)).WillRepeatedly(Return(true)); auto context = MakeContext(ast, symbol_table, &router, &id_alloc); auto multi_frame = CreateMultiFrame(context.symbol_table.max_position()); cursor->PullMultiple(multi_frame, context);