From 30a4f40093aa75250932a7b57247902c46c6458d Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Wed, 17 May 2017 12:15:24 +0200 Subject: [PATCH] Skip Null in SET and REMOVE clauses Summary: openCypher expects removing/setting properties and labels on Null vertices/edges does not produce an error. Instead, Nulls are simply skipped. Reviewers: florijan, mislav.bradac, buda Reviewed By: mislav.bradac Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D375 --- src/query/plan/operator.cpp | 15 ++++ .../query_plan_create_set_remove_delete.cpp | 82 +++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index b835da410..930509e68 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -513,6 +513,9 @@ bool SetProperty::SetPropertyCursor::Pull(Frame &frame, case TypedValue::Type::Edge: lhs.Value().PropsSet(self_.lhs_->property_, rhs); break; + case TypedValue::Type::Null: + // Skip setting properties on Null (can occur in optional match). + break; default: throw QueryRuntimeException( "Properties can only be set on Vertices and Edges"); @@ -553,6 +556,9 @@ bool SetProperties::SetPropertiesCursor::Pull(Frame &frame, case TypedValue::Type::Edge: Set(lhs.Value(), rhs); break; + case TypedValue::Type::Null: + // Skip setting properties on Null (can occur in optional match). + break; default: throw QueryRuntimeException( "Properties can only be set on Vertices and Edges"); @@ -620,6 +626,9 @@ bool SetLabels::SetLabelsCursor::Pull(Frame &frame, if (!input_cursor_->Pull(frame, symbol_table)) return false; TypedValue &vertex_value = frame[self_.input_symbol_]; + // Skip setting labels on Null (can occur in optional match). + if (vertex_value.IsNull()) return true; + auto &vertex = vertex_value.Value(); vertex.SwitchNew(); for (auto label : self_.labels_) vertex.add_label(label); @@ -658,6 +667,9 @@ bool RemoveProperty::RemovePropertyCursor::Pull( case TypedValue::Type::Edge: lhs.Value().PropsErase(self_.lhs_->property_); break; + case TypedValue::Type::Null: + // Skip removing properties on Null (can occur in optional match). + break; default: // TODO consider throwing a TypedValueException here // deal with this when we'll be overhauling error-feedback @@ -689,6 +701,9 @@ bool RemoveLabels::RemoveLabelsCursor::Pull(Frame &frame, if (!input_cursor_->Pull(frame, symbol_table)) return false; TypedValue &vertex_value = frame[self_.input_symbol_]; + // Skip removing labels on Null (can occur in optional match). + if (vertex_value.IsNull()) return true; + auto &vertex = vertex_value.Value(); vertex.SwitchNew(); for (auto label : self_.labels_) vertex.remove_label(label); diff --git a/tests/unit/query_plan_create_set_remove_delete.cpp b/tests/unit/query_plan_create_set_remove_delete.cpp index a9c3ae003..3cda689d7 100644 --- a/tests/unit/query_plan_create_set_remove_delete.cpp +++ b/tests/unit/query_plan_create_set_remove_delete.cpp @@ -854,3 +854,85 @@ TEST(QueryPlan, MergeNoInput) { dba->advance_command(); EXPECT_EQ(1, CountIterable(dba->vertices())); } + +TEST(QueryPlan, SetPropertyOnNull) { + // SET (Null).prop = 42 + Dbms dbms; + auto dba = dbms.active(); + AstTreeStorage storage; + SymbolTable symbol_table; + auto prop = dba->property("prop"); + auto null = LITERAL(TypedValue::Null); + auto literal = LITERAL(42); + auto n_prop = storage.Create(null, prop); + auto once = std::make_shared(); + auto set_op = std::make_shared(once, n_prop, literal); + EXPECT_EQ(1, PullAll(set_op, *dba, symbol_table)); +} + +TEST(QueryPlan, SetPropertiesOnNull) { + // OPTIONAL MATCH (n) SET n = n + Dbms dbms; + auto dba = dbms.active(); + AstTreeStorage storage; + SymbolTable symbol_table; + auto n = MakeScanAll(storage, symbol_table, "n"); + auto n_ident = IDENT("n"); + symbol_table[*n_ident] = n.sym_; + auto optional = std::make_shared(nullptr, n.op_, + std::vector{n.sym_}); + auto set_op = std::make_shared( + optional, n.sym_, n_ident, plan::SetProperties::Op::REPLACE); + EXPECT_EQ(0, CountIterable(dba->vertices())); + EXPECT_EQ(1, PullAll(set_op, *dba, symbol_table)); +} + +TEST(QueryPlan, SetLabelsOnNull) { + // OPTIONAL MATCH (n) SET n :label + Dbms dbms; + auto dba = dbms.active(); + auto label = dba->label("label"); + AstTreeStorage storage; + SymbolTable symbol_table; + auto n = MakeScanAll(storage, symbol_table, "n"); + auto n_ident = IDENT("n"); + symbol_table[*n_ident] = n.sym_; + auto optional = std::make_shared(nullptr, n.op_, + std::vector{n.sym_}); + auto set_op = std::make_shared( + optional, n.sym_, std::vector{label}); + EXPECT_EQ(0, CountIterable(dba->vertices())); + EXPECT_EQ(1, PullAll(set_op, *dba, symbol_table)); +} + +TEST(QueryPlan, RemovePropertyOnNull) { + // REMOVE (Null).prop + Dbms dbms; + auto dba = dbms.active(); + AstTreeStorage storage; + SymbolTable symbol_table; + auto prop = dba->property("prop"); + auto null = LITERAL(TypedValue::Null); + auto n_prop = storage.Create(null, prop); + auto once = std::make_shared(); + auto remove_op = std::make_shared(once, n_prop); + EXPECT_EQ(1, PullAll(remove_op, *dba, symbol_table)); +} + +TEST(QueryPlan, RemoveLabelsOnNull) { + // OPTIONAL MATCH (n) REMOVE n :label + Dbms dbms; + auto dba = dbms.active(); + auto label = dba->label("label"); + AstTreeStorage storage; + SymbolTable symbol_table; + auto n = MakeScanAll(storage, symbol_table, "n"); + auto n_ident = IDENT("n"); + symbol_table[*n_ident] = n.sym_; + auto optional = std::make_shared(nullptr, n.op_, + std::vector{n.sym_}); + auto remove_op = std::make_shared( + optional, n.sym_, std::vector{label}); + EXPECT_EQ(0, CountIterable(dba->vertices())); + EXPECT_EQ(1, PullAll(remove_op, *dba, symbol_table)); +}