From 5134a94f1c8a3bc12c535e52166a468051db3205 Mon Sep 17 00:00:00 2001 From: florijan Date: Thu, 30 Mar 2017 08:38:48 +0200 Subject: [PATCH] Query::Plan - RemoveLabels added and tested Summary: Query::Plan::RemoveProperty op added and tested Query::AST:: Remove added to cypher main visitor. Tested. Grammar corrected on missing whitespace in removeItem production Reviewers: buda, mislav.bradac, teon.banek Reviewed By: mislav.bradac Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D202 --- src/query/frontend/ast/ast.hpp | 36 ++++++ src/query/frontend/ast/ast_visitor.hpp | 4 +- .../frontend/ast/cypher_main_visitor.cpp | 30 +++++ .../frontend/ast/cypher_main_visitor.hpp | 10 ++ src/query/frontend/logical/operator.hpp | 113 +++++++++++++++++- .../frontend/opencypher/grammar/Cypher.g4 | 2 +- tests/unit/cypher_main_visitor.cpp | 27 +++++ tests/unit/interpreter.cpp | 83 +++++++++++++ 8 files changed, 302 insertions(+), 3 deletions(-) diff --git a/src/query/frontend/ast/ast.hpp b/src/query/frontend/ast/ast.hpp index 660f50995..f64e6a15f 100644 --- a/src/query/frontend/ast/ast.hpp +++ b/src/query/frontend/ast/ast.hpp @@ -631,6 +631,42 @@ class SetLabels : public Clause { : Clause(uid), identifier_(identifier), labels_(labels) {} }; +class RemoveProperty : public Clause { + friend class AstTreeStorage; + + public: + void Accept(TreeVisitorBase &visitor) override { + visitor.Visit(*this); + property_lookup_->Accept(visitor); + visitor.PostVisit(*this); + } + PropertyLookup *property_lookup_ = nullptr; + + protected: + RemoveProperty(int uid) : Clause(uid) {} + RemoveProperty(int uid, PropertyLookup *property_lookup) + : Clause(uid), property_lookup_(property_lookup) {} +}; + +class RemoveLabels : public Clause { + friend class AstTreeStorage; + + public: + void Accept(TreeVisitorBase &visitor) override { + visitor.Visit(*this); + identifier_->Accept(visitor); + visitor.PostVisit(*this); + } + Identifier *identifier_ = nullptr; + std::vector labels_; + + protected: + RemoveLabels(int uid) : Clause(uid) {} + RemoveLabels(int uid, Identifier *identifier, + const std::vector &labels) + : Clause(uid), identifier_(identifier), labels_(labels) {} +}; + // It would be better to call this AstTree, but we already have a class Tree, // which could be renamed to Node or AstTreeNode, but we also have a class // called NodeAtom... diff --git a/src/query/frontend/ast/ast_visitor.hpp b/src/query/frontend/ast/ast_visitor.hpp index 2e3ad504d..27024e70b 100644 --- a/src/query/frontend/ast/ast_visitor.hpp +++ b/src/query/frontend/ast/ast_visitor.hpp @@ -38,6 +38,8 @@ class Where; class SetProperty; class SetProperties; class SetLabels; +class RemoveProperty; +class RemoveLabels; using TreeVisitorBase = ::utils::Visitor< Query, NamedExpression, OrOperator, XorOperator, AndOperator, NotOperator, @@ -46,5 +48,5 @@ using TreeVisitorBase = ::utils::Visitor< LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator, UnaryPlusOperator, UnaryMinusOperator, Identifier, Literal, PropertyLookup, Create, Match, Return, Pattern, NodeAtom, EdgeAtom, Delete, Where, - SetProperty, SetProperties, SetLabels>; + SetProperty, SetProperties, SetLabels, RemoveProperty, RemoveLabels>; } diff --git a/src/query/frontend/ast/cypher_main_visitor.cpp b/src/query/frontend/ast/cypher_main_visitor.cpp index e3071f702..1b4a8bc5a 100644 --- a/src/query/frontend/ast/cypher_main_visitor.cpp +++ b/src/query/frontend/ast/cypher_main_visitor.cpp @@ -65,6 +65,10 @@ antlrcpp::Any CypherMainVisitor::visitClause(CypherParser::ClauseContext *ctx) { // Different return type!!! return ctx->set()->accept(this).as>(); } + if (ctx->remove()) { + // Different return type!!! + return ctx->remove()->accept(this).as>(); + } // TODO: implement other clauses. throw NotYetImplemented(); return 0; @@ -744,6 +748,32 @@ antlrcpp::Any CypherMainVisitor::visitSetItem( return static_cast(set_labels); } +antlrcpp::Any CypherMainVisitor::visitRemove(CypherParser::RemoveContext *ctx) { + std::vector remove_items; + for (auto *remove_item : ctx->removeItem()) { + remove_items.push_back(remove_item->accept(this)); + } + return remove_items; +} + +antlrcpp::Any CypherMainVisitor::visitRemoveItem( + CypherParser::RemoveItemContext *ctx) { + // RemoveProperty + if (ctx->propertyExpression()) { + auto *remove_property = storage_.Create(); + remove_property->property_lookup_ = ctx->propertyExpression()->accept(this); + return static_cast(remove_property); + } + + // RemoveLabels + auto *remove_labels = storage_.Create(); + remove_labels->identifier_ = storage_.Create( + ctx->variable()->accept(this).as()); + remove_labels->labels_ = + ctx->nodeLabels()->accept(this).as>(); + return static_cast(remove_labels); +} + antlrcpp::Any CypherMainVisitor::visitPropertyExpression( CypherParser::PropertyExpressionContext *ctx) { Expression *expression = ctx->atom()->accept(this); diff --git a/src/query/frontend/ast/cypher_main_visitor.hpp b/src/query/frontend/ast/cypher_main_visitor.hpp index 4742d074f..559790904 100644 --- a/src/query/frontend/ast/cypher_main_visitor.hpp +++ b/src/query/frontend/ast/cypher_main_visitor.hpp @@ -421,6 +421,16 @@ class CypherMainVisitor : public antlropencypher::CypherBaseVisitor { */ antlrcpp::Any visitSetItem(CypherParser::SetItemContext *ctx) override; + /** + * return vector + */ + antlrcpp::Any visitRemove(CypherParser::RemoveContext *ctx) override; + + /** + * @return Clause* + */ + antlrcpp::Any visitRemoveItem(CypherParser::RemoveItemContext *ctx) override; + /** * @return PropertyLookup* */ diff --git a/src/query/frontend/logical/operator.hpp b/src/query/frontend/logical/operator.hpp index be93b4b1d..c1a4ac820 100644 --- a/src/query/frontend/logical/operator.hpp +++ b/src/query/frontend/logical/operator.hpp @@ -34,11 +34,13 @@ class Delete; class SetProperty; class SetProperties; class SetLabels; +class RemoveProperty; +class RemoveLabels; using LogicalOperatorVisitor = ::utils::Visitor; + SetProperties, SetLabels, RemoveProperty, RemoveLabels>; class LogicalOperator : public ::utils::Visitable { public: @@ -1041,5 +1043,114 @@ class SetLabels : public LogicalOperator { std::vector labels_; }; +/** + * Logical op for removing a property from an + * edge or a vertex. + */ +class RemoveProperty : public LogicalOperator { + public: + RemoveProperty(const std::shared_ptr input, + PropertyLookup *lhs) + : input_(input), lhs_(lhs) {} + + void Accept(LogicalOperatorVisitor &visitor) override { + visitor.Visit(*this); + input_->Accept(visitor); + visitor.PostVisit(*this); + } + + private: + class RemovePropertyCursor : public Cursor { + public: + RemovePropertyCursor(RemoveProperty &self, GraphDbAccessor &db) + : self_(self), input_cursor_(self.input_->MakeCursor(db)) {} + + bool Pull(Frame &frame, SymbolTable &symbol_table) override { + if (!input_cursor_->Pull(frame, symbol_table)) return false; + + ExpressionEvaluator evaluator(frame, symbol_table); + self_.lhs_->expression_->Accept(evaluator); + TypedValue lhs = evaluator.PopBack(); + + switch (lhs.type()) { + case TypedValue::Type::Vertex: + lhs.Value().PropsErase(self_.lhs_->property_); + break; + case TypedValue::Type::Edge: + lhs.Value().PropsErase(self_.lhs_->property_); + break; + default: + // TODO consider throwing a TypedValueException here + // deal with this when we'll be overhauling error-feedback + throw QueryRuntimeException( + "Properties can only be removed on Vertices and Edges"); + } + return true; + } + + private: + RemoveProperty &self_; + std::unique_ptr input_cursor_; + }; + + public: + std::unique_ptr MakeCursor(GraphDbAccessor &db) override { + return std::make_unique(*this, db); + } + + private: + std::shared_ptr input_; + PropertyLookup *lhs_; +}; +/** + * Logical operator for removing an arbitrary number of + * labels on a Vertex. If a label does not exist on a Vertex, + * nothing happens. + */ +class RemoveLabels : public LogicalOperator { + public: + RemoveLabels(const std::shared_ptr input, + const Symbol input_symbol, + const std::vector &labels) + : input_(input), input_symbol_(input_symbol), labels_(labels) {} + + void Accept(LogicalOperatorVisitor &visitor) override { + visitor.Visit(*this); + input_->Accept(visitor); + visitor.PostVisit(*this); + } + + private: + class RemoveLabelsCursor : public Cursor { + public: + RemoveLabelsCursor(RemoveLabels &self, GraphDbAccessor &db) + : self_(self), input_cursor_(self.input_->MakeCursor(db)) {} + + bool Pull(Frame &frame, SymbolTable &symbol_table) override { + if (!input_cursor_->Pull(frame, symbol_table)) return false; + + TypedValue vertex_value = frame[self_.input_symbol_]; + VertexAccessor vertex = vertex_value.Value(); + for (auto label : self_.labels_) vertex.remove_label(label); + + return true; + } + + private: + RemoveLabels &self_; + std::unique_ptr input_cursor_; + }; + + public: + std::unique_ptr MakeCursor(GraphDbAccessor &db) override { + return std::make_unique(*this, db); + } + + private: + std::shared_ptr input_; + const Symbol input_symbol_; + std::vector labels_; +}; + } // namespace plan } // namespace query diff --git a/src/query/frontend/opencypher/grammar/Cypher.g4 b/src/query/frontend/opencypher/grammar/Cypher.g4 index e5137604f..80e808948 100644 --- a/src/query/frontend/opencypher/grammar/Cypher.g4 +++ b/src/query/frontend/opencypher/grammar/Cypher.g4 @@ -65,7 +65,7 @@ cypherDelete : ( DETACH SP )? DELETE SP? expression ( SP? ',' SP? expression )* remove : REMOVE SP removeItem ( SP? ',' SP? removeItem )* ; -removeItem : ( variable nodeLabels ) +removeItem : ( variable SP? nodeLabels ) | propertyExpression ; diff --git a/tests/unit/cypher_main_visitor.cpp b/tests/unit/cypher_main_visitor.cpp index 635041e96..67ec4a092 100644 --- a/tests/unit/cypher_main_visitor.cpp +++ b/tests/unit/cypher_main_visitor.cpp @@ -669,4 +669,31 @@ TEST(Visitor, Set) { ast_generator.db_accessor_->label("i"))); } } + +TEST(Visitor, Remove) { + AstGenerator ast_generator("REMOVE a.x, g : h : i"); + auto *query = ast_generator.query_; + ASSERT_EQ(query->clauses_.size(), 2U); + + { + auto *remove_property = dynamic_cast(query->clauses_[0]); + ASSERT_TRUE(remove_property); + ASSERT_TRUE(remove_property->property_lookup_); + auto *identifier1 = dynamic_cast( + remove_property->property_lookup_->expression_); + ASSERT_TRUE(identifier1); + ASSERT_EQ(identifier1->name_, "a"); + ASSERT_EQ(remove_property->property_lookup_->property_, + ast_generator.db_accessor_->property("x")); + } + { + auto *remove_labels = dynamic_cast(query->clauses_[1]); + ASSERT_TRUE(remove_labels); + ASSERT_TRUE(remove_labels->identifier_); + ASSERT_EQ(remove_labels->identifier_->name_, "g"); + ASSERT_THAT(remove_labels->labels_, + UnorderedElementsAre(ast_generator.db_accessor_->label("h"), + ast_generator.db_accessor_->label("i"))); + } +} } diff --git a/tests/unit/interpreter.cpp b/tests/unit/interpreter.cpp index 3f3cb3caf..513b8951f 100644 --- a/tests/unit/interpreter.cpp +++ b/tests/unit/interpreter.cpp @@ -1004,3 +1004,86 @@ TEST(Interpreter, SetLabels) { EXPECT_TRUE(vertex.has_label(label3)); } } + +TEST(Interpreter, RemoveProperty) { + Dbms dbms; + auto dba = dbms.active(); + + // graph with 4 vertices in connected pairs + // the origin vertex in each par and both edges + // have a property set + auto prop1 = dba->property("prop1"); + auto v1 = dba->insert_vertex(); + auto v2 = dba->insert_vertex(); + auto v3 = dba->insert_vertex(); + auto v4 = dba->insert_vertex(); + auto edge_type = dba->edge_type("edge_type"); + dba->insert_edge(v1, v3, edge_type).PropsSet(prop1, 42); + dba->insert_edge(v2, v4, edge_type); + v2.PropsSet(prop1, 42); + v3.PropsSet(prop1, 42); + v4.PropsSet(prop1, 42); + auto prop2 = dba->property("prop2"); + v1.PropsSet(prop2, 0); + v2.PropsSet(prop2, 0); + dba->advance_command(); + + AstTreeStorage storage; + SymbolTable symbol_table; + + // scan (n)-[r]->(m) + auto n = MakeScanAll(storage, symbol_table, "n"); + auto r_m = MakeExpand(storage, symbol_table, n.op_, n.sym_, "r", + EdgeAtom::Direction::RIGHT, false, "m", false); + + auto n_p = PROPERTY_LOOKUP("n", prop1); + symbol_table[*n_p->expression_] = n.sym_; + auto set_n_p = std::make_shared(r_m.op_, n_p); + + auto r_p = PROPERTY_LOOKUP("r", prop1); + symbol_table[*r_p->expression_] = r_m.edge_sym_; + auto set_r_p = std::make_shared(set_n_p, r_p); + EXPECT_EQ(2, PullAll(set_r_p, *dba, symbol_table)); + dba->advance_command(); + + EXPECT_EQ(CountIterable(dba->edges()), 2); + for (EdgeAccessor edge : dba->edges()) { + EXPECT_EQ(edge.PropsAt(prop1).type(), PropertyValue::Type::Null); + VertexAccessor from = edge.from(); + VertexAccessor to = edge.to(); + EXPECT_EQ(from.PropsAt(prop1).type(), PropertyValue::Type::Null); + EXPECT_EQ(from.PropsAt(prop2).type(), PropertyValue::Type::Int); + EXPECT_EQ(to.PropsAt(prop1).type(), PropertyValue::Type::Int); + } +} + +TEST(Interpreter, RemoveLabels) { + Dbms dbms; + auto dba = dbms.active(); + + auto label1 = dba->label("label1"); + auto label2 = dba->label("label2"); + auto label3 = dba->label("label3"); + auto v1 = dba->insert_vertex(); + v1.add_label(label1); + v1.add_label(label2); + v1.add_label(label3); + auto v2 = dba->insert_vertex(); + v2.add_label(label1); + v2.add_label(label3); + dba->advance_command(); + + AstTreeStorage storage; + SymbolTable symbol_table; + + auto n = MakeScanAll(storage, symbol_table, "n"); + auto label_remove = std::make_shared( + n.op_, n.sym_, std::vector{label1, label2}); + EXPECT_EQ(2, PullAll(label_remove, *dba, symbol_table)); + + for (VertexAccessor vertex : dba->vertices()) { + EXPECT_EQ(1, vertex.labels().size()); + EXPECT_FALSE(vertex.has_label(label1)); + EXPECT_FALSE(vertex.has_label(label2)); + } +}