From 8bfebfca9db0488987bb96c8ea1a5514b0f46a10 Mon Sep 17 00:00:00 2001 From: jseljan Date: Fri, 19 Jun 2020 16:47:56 +0200 Subject: [PATCH] Add 'none' function implementation, its unit and TCK tests Reviewers: mferencevic Reviewed By: mferencevic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2788 --- src/query/frontend/ast/ast.lcp | 49 +++++++++++- src/query/frontend/ast/ast_visitor.hpp | 5 +- .../frontend/ast/cypher_main_visitor.cpp | 14 ++++ src/query/frontend/ast/pretty_print.cpp | 6 ++ .../frontend/semantic/symbol_generator.cpp | 6 ++ .../frontend/semantic/symbol_generator.hpp | 1 + src/query/interpret/eval.hpp | 35 +++++++++ src/query/plan/preprocess.hpp | 7 ++ src/query/plan/rule_based_planner.cpp | 15 ++++ .../memgraph_V1/features/functions.feature | 74 ++++++++++++++++++- tests/unit/query_common.hpp | 3 + tests/unit/query_expression_evaluator.cpp | 68 +++++++++++++++++ 12 files changed, 276 insertions(+), 7 deletions(-) diff --git a/src/query/frontend/ast/ast.lcp b/src/query/frontend/ast/ast.lcp index 70f8678cb..5114648a1 100644 --- a/src/query/frontend/ast/ast.lcp +++ b/src/query/frontend/ast/ast.lcp @@ -988,8 +988,8 @@ cpp<# (:clone)) ;; TODO: This is pretty much copy pasted from All. Consider merging Reduce, -;; All, Any and Single into something like a higher-order function call which -;; takes a list argument and a function which is applied on list elements. +;; All, Any, None and Single into something like a higher-order function call +;; which takes a list argument and a function which is applied on list elements. (lcp:define-class single (expression) ((identifier "Identifier *" :initval "nullptr" :scope :public :slk-save #'slk-save-ast-pointer @@ -1029,8 +1029,8 @@ cpp<# (:clone)) ;; TODO: This is pretty much copy pasted from All. Consider merging Reduce, -;; All, Any and Single into something like a higher-order function call which -;; takes a list argument and a function which is applied on list elements. +;; All, Any, None and Single into something like a higher-order function call +;; which takes a list argument and a function which is applied on list elements. (lcp:define-class any (expression) ((identifier "Identifier *" :initval "nullptr" :scope :public :slk-save #'slk-save-ast-pointer @@ -1069,6 +1069,47 @@ cpp<# (:serialize (:slk)) (:clone)) +;; TODO: This is pretty much copy pasted from All. Consider merging Reduce, +;; All, Any, None and Single into something like a higher-order function call +;; which takes a list argument and a function which is applied on list elements. +(lcp:define-class none (expression) + ((identifier "Identifier *" :initval "nullptr" :scope :public + :slk-save #'slk-save-ast-pointer + :slk-load (slk-load-ast-pointer "Identifier")) + (list-expression "Expression *" :initval "nullptr" :scope :public + :slk-save #'slk-save-ast-pointer + :slk-load (slk-load-ast-pointer "Expression")) + (where "Where *" :initval "nullptr" :scope :public + :slk-save #'slk-save-ast-pointer + :slk-load (slk-load-ast-pointer "Where"))) + (:public + #>cpp + None() = default; + + DEFVISITABLE(ExpressionVisitor); + DEFVISITABLE(ExpressionVisitor); + bool Accept(HierarchicalTreeVisitor &visitor) override { + if (visitor.PreVisit(*this)) { + identifier_->Accept(visitor) && list_expression_->Accept(visitor) && + where_->Accept(visitor); + } + return visitor.PostVisit(*this); + } + cpp<#) + (:protected + #>cpp + None(Identifier *identifier, Expression *list_expression, Where *where) + : identifier_(identifier), + list_expression_(list_expression), + where_(where) {} + cpp<#) + (:private + #>cpp + friend class AstStorage; + cpp<#) + (:serialize (:slk)) + (:clone)) + (lcp:define-class parameter-lookup (expression) ((token-position :int32_t :initval -1 :scope :public :documentation "This field contains token position of *literal* used to create ParameterLookup object. If ParameterLookup object is not created from a literal leave this value at -1.")) diff --git a/src/query/frontend/ast/ast_visitor.hpp b/src/query/frontend/ast/ast_visitor.hpp index ffc6229fd..3eb6632db 100644 --- a/src/query/frontend/ast/ast_visitor.hpp +++ b/src/query/frontend/ast/ast_visitor.hpp @@ -20,6 +20,7 @@ class Extract; class All; class Single; class Any; +class None; class ParameterLookup; class CallProcedure; class Create; @@ -80,7 +81,7 @@ using TreeCompositeVisitor = ::utils::CompositeVisitor< GreaterEqualOperator, InListOperator, SubscriptOperator, ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral, MapLiteral, PropertyLookup, LabelsTest, - Aggregation, Function, Reduce, Coalesce, Extract, All, Single, Any, + Aggregation, Function, Reduce, Coalesce, Extract, All, Single, Any, None, CallProcedure, Create, Match, Return, With, Pattern, NodeAtom, EdgeAtom, Delete, Where, SetProperty, SetProperties, SetLabels, RemoveProperty, RemoveLabels, Merge, Unwind, RegexMatch>; @@ -108,7 +109,7 @@ class ExpressionVisitor SubscriptOperator, ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral, MapLiteral, PropertyLookup, LabelsTest, Aggregation, Function, Reduce, Coalesce, - Extract, All, Single, Any, ParameterLookup, Identifier, + Extract, All, Single, Any, None, ParameterLookup, Identifier, PrimitiveLiteral, RegexMatch> {}; template diff --git a/src/query/frontend/ast/cypher_main_visitor.cpp b/src/query/frontend/ast/cypher_main_visitor.cpp index 9e08779b7..f4462a664 100644 --- a/src/query/frontend/ast/cypher_main_visitor.cpp +++ b/src/query/frontend/ast/cypher_main_visitor.cpp @@ -1391,6 +1391,20 @@ antlrcpp::Any CypherMainVisitor::visitAtom(MemgraphCypher::AtomContext *ctx) { Where *where = ctx->filterExpression()->where()->accept(this); return static_cast( storage_->Create(ident, list_expr, where)); + } else if (ctx->NONE()) { + auto *ident = storage_->Create(ctx->filterExpression() + ->idInColl() + ->variable() + ->accept(this) + .as()); + Expression *list_expr = + ctx->filterExpression()->idInColl()->expression()->accept(this); + if (!ctx->filterExpression()->where()) { + throw SyntaxException("NONE(...) requires a WHERE predicate."); + } + Where *where = ctx->filterExpression()->where()->accept(this); + return static_cast( + storage_->Create(ident, list_expr, where)); } else if (ctx->REDUCE()) { auto *accumulator = storage_->Create( ctx->reduceExpression()->accumulator->accept(this).as()); diff --git a/src/query/frontend/ast/pretty_print.cpp b/src/query/frontend/ast/pretty_print.cpp index 26320ee18..c46e7a716 100644 --- a/src/query/frontend/ast/pretty_print.cpp +++ b/src/query/frontend/ast/pretty_print.cpp @@ -52,6 +52,7 @@ class ExpressionPrettyPrinter : public ExpressionVisitor { void Visit(All &op) override; void Visit(Single &op) override; void Visit(Any &op) override; + void Visit(None &op) override; void Visit(Identifier &op) override; void Visit(PrimitiveLiteral &op) override; void Visit(PropertyLookup &op) override; @@ -292,6 +293,11 @@ void ExpressionPrettyPrinter::Visit(Any &op) { op.where_->expression_); } +void ExpressionPrettyPrinter::Visit(None &op) { + PrintOperator(out_, "None", op.identifier_, op.list_expression_, + op.where_->expression_); +} + void ExpressionPrettyPrinter::Visit(Identifier &op) { PrintOperator(out_, "Identifier", op.name_); } diff --git a/src/query/frontend/semantic/symbol_generator.cpp b/src/query/frontend/semantic/symbol_generator.cpp index b88c36b20..a049a425b 100644 --- a/src/query/frontend/semantic/symbol_generator.cpp +++ b/src/query/frontend/semantic/symbol_generator.cpp @@ -360,6 +360,12 @@ bool SymbolGenerator::PreVisit(Any &any) { return false; } +bool SymbolGenerator::PreVisit(None &none) { + none.list_expression_->Accept(*this); + VisitWithIdentifiers(none.where_->expression_, {none.identifier_}); + return false; +} + bool SymbolGenerator::PreVisit(Reduce &reduce) { reduce.initializer_->Accept(*this); reduce.list_->Accept(*this); diff --git a/src/query/frontend/semantic/symbol_generator.hpp b/src/query/frontend/semantic/symbol_generator.hpp index 103febb11..21aa71a79 100644 --- a/src/query/frontend/semantic/symbol_generator.hpp +++ b/src/query/frontend/semantic/symbol_generator.hpp @@ -59,6 +59,7 @@ class SymbolGenerator : public HierarchicalTreeVisitor { bool PreVisit(All &) override; bool PreVisit(Single &) override; bool PreVisit(Any &) override; + bool PreVisit(None &) override; bool PreVisit(Reduce &) override; bool PreVisit(Extract &) override; diff --git a/src/query/interpret/eval.hpp b/src/query/interpret/eval.hpp index e5a7acfd6..c98422154 100644 --- a/src/query/interpret/eval.hpp +++ b/src/query/interpret/eval.hpp @@ -558,6 +558,41 @@ class ExpressionEvaluator : public ExpressionVisitor { } } + TypedValue Visit(None &none) override { + auto list_value = none.list_expression_->Accept(*this); + if (list_value.IsNull()) { + return TypedValue(ctx_->memory); + } + if (list_value.type() != TypedValue::Type::List) { + throw QueryRuntimeException("NONE expected a list, got {}.", + list_value.type()); + } + const auto &list = list_value.ValueList(); + const auto &symbol = symbol_table_->at(*none.identifier_); + bool has_value = false; + for (const auto &element : list) { + frame_->at(symbol) = element; + auto result = none.where_->expression_->Accept(*this); + if (!result.IsNull() && result.type() != TypedValue::Type::Bool) { + throw QueryRuntimeException( + "Predicate of NONE must evaluate to boolean, got {}.", + result.type()); + } + if (!result.IsNull()) { + has_value = true; + if (result.ValueBool()) { + return TypedValue(false, ctx_->memory); + } + } + } + // Return Null if all elements are Null + if (!has_value) { + return TypedValue(ctx_->memory); + } else { + return TypedValue(true, ctx_->memory); + } + } + TypedValue Visit(ParameterLookup ¶m_lookup) override { return TypedValue( ctx_->parameters.AtTokenPosition(param_lookup.token_position_), diff --git a/src/query/plan/preprocess.hpp b/src/query/plan/preprocess.hpp index dcd4cacb4..14a7bf37a 100644 --- a/src/query/plan/preprocess.hpp +++ b/src/query/plan/preprocess.hpp @@ -44,6 +44,13 @@ class UsedSymbolsCollector : public HierarchicalTreeVisitor { return true; } + bool PostVisit(None &none) override { + // Remove the symbol which is bound by none, because we are only interested + // in free (unbound) symbols. + symbols_.erase(symbol_table_.at(*none.identifier_)); + return true; + } + bool PostVisit(Reduce &reduce) override { // Remove the symbols bound by reduce, because we are only interested // in free (unbound) symbols. diff --git a/src/query/plan/rule_based_planner.cpp b/src/query/plan/rule_based_planner.cpp index 3d90a5fee..08cdb7a9d 100644 --- a/src/query/plan/rule_based_planner.cpp +++ b/src/query/plan/rule_based_planner.cpp @@ -180,6 +180,21 @@ class ReturnBodyContext : public HierarchicalTreeVisitor { return true; } + bool PostVisit(None &none) override { + // Remove the symbol which is bound by none, because we are only interested + // in free (unbound) symbols. + used_symbols_.erase(symbol_table_.at(*none.identifier_)); + CHECK(has_aggregation_.size() >= 3U) + << "Expected 3 has_aggregation_ flags for NONE arguments"; + bool has_aggr = false; + for (int i = 0; i < 3; ++i) { + has_aggr = has_aggr || has_aggregation_.back(); + has_aggregation_.pop_back(); + } + has_aggregation_.emplace_back(has_aggr); + return true; + } + bool PostVisit(Reduce &reduce) override { // Remove the symbols bound by reduce, because we are only interested // in free (unbound) symbols. diff --git a/tests/qa/tests/memgraph_V1/features/functions.feature b/tests/qa/tests/memgraph_V1/features/functions.feature index 659cf2bfb..461fcfd7e 100644 --- a/tests/qa/tests/memgraph_V1/features/functions.feature +++ b/tests/qa/tests/memgraph_V1/features/functions.feature @@ -785,7 +785,79 @@ Feature: Functions | a | | null | - + Scenario: None test 01: + When executing query: + """ + RETURN none(x IN [1, 2, 3] WHERE x < 1) AS a + """ + Then the result should be: + | a | + | true | + + Scenario: None test 02: + When executing query: + """ + RETURN none(x IN [1, 2, 3] WHERE x = 1) AS a + """ + Then the result should be: + | a | + | false | + + Scenario: None test 03: + When executing query: + """ + RETURN none(x IN ["a", "b", "c"] WHERE x = 1) AS a + """ + Then the result should be: + | a | + | true | + + Scenario: None test 04: + When executing query: + """ + RETURN none(x IN [Null, Null, Null] WHERE x = 0) AS a + """ + Then the result should be: + | a | + | null | + + Scenario: None test 05: + When executing query: + """ + RETURN none(x IN [Null, Null, 0] WHERE x > 0) AS a + """ + Then the result should be: + | a | + | true | + + Scenario: None test 06: + When executing query: + """ + RETURN none(x IN [Null, Null, 0] WHERE x = 0) AS a + """ + Then the result should be: + | a | + | false | + + Scenario: None test 07: + When executing query: + """ + RETURN none(x IN [Null, Null, Null] WHERE x = Null) AS a + """ + Then the result should be: + | a | + | null | + + Scenario: None test 08: + When executing query: + """ + RETURN none(x IN ["a", "b", "c"] WHERE x = Null) AS a + """ + Then the result should be: + | a | + | null | + + Scenario: Reduce test 01: When executing query: """ diff --git a/tests/unit/query_common.hpp b/tests/unit/query_common.hpp index bd63a14c5..324575dea 100644 --- a/tests/unit/query_common.hpp +++ b/tests/unit/query_common.hpp @@ -642,6 +642,9 @@ auto GetMerge(AstStorage &storage, Pattern *pattern, OnMatch on_match, #define ANY(variable, list, where) \ storage.Create(storage.Create(variable), \ list, where) +#define NONE(variable, list, where) \ + storage.Create(storage.Create(variable), \ + list, where) #define REDUCE(accumulator, initializer, variable, list, expr) \ storage.Create( \ storage.Create(accumulator), initializer, \ diff --git a/tests/unit/query_expression_evaluator.cpp b/tests/unit/query_expression_evaluator.cpp index 433bc525e..2230170a9 100644 --- a/tests/unit/query_expression_evaluator.cpp +++ b/tests/unit/query_expression_evaluator.cpp @@ -836,6 +836,74 @@ TEST_F(ExpressionEvaluatorTest, FunctionAnyWhereWrongType) { EXPECT_THROW(Eval(any), QueryRuntimeException); } +TEST_F(ExpressionEvaluatorTest, FunctionNone1) { + AstStorage storage; + auto *ident_x = IDENT("x"); + auto *none = + NONE("x", LIST(LITERAL(1), LITERAL(2)), WHERE(EQ(ident_x, LITERAL(0)))); + const auto x_sym = symbol_table.CreateSymbol("x", true); + none->identifier_->MapTo(x_sym); + ident_x->MapTo(x_sym); + auto value = Eval(none); + ASSERT_TRUE(value.IsBool()); + EXPECT_TRUE(value.ValueBool()); +} + +TEST_F(ExpressionEvaluatorTest, FunctionNone2) { + AstStorage storage; + auto *ident_x = IDENT("x"); + auto *none = + NONE("x", LIST(LITERAL(1), LITERAL(2)), WHERE(EQ(ident_x, LITERAL(1)))); + const auto x_sym = symbol_table.CreateSymbol("x", true); + none->identifier_->MapTo(x_sym); + ident_x->MapTo(x_sym); + auto value = Eval(none); + ASSERT_TRUE(value.IsBool()); + EXPECT_FALSE(value.ValueBool()); +} + +TEST_F(ExpressionEvaluatorTest, FunctionNoneNullList) { + AstStorage storage; + auto *none = + NONE("x", LITERAL(storage::PropertyValue()), WHERE(LITERAL(true))); + const auto x_sym = symbol_table.CreateSymbol("x", true); + none->identifier_->MapTo(x_sym); + auto value = Eval(none); + EXPECT_TRUE(value.IsNull()); +} + +TEST_F(ExpressionEvaluatorTest, FunctionNoneNullElementInList1) { + AstStorage storage; + auto *ident_x = IDENT("x"); + auto *any = NONE("x", LIST(LITERAL(1), LITERAL(storage::PropertyValue())), + WHERE(EQ(ident_x, LITERAL(0)))); + const auto x_sym = symbol_table.CreateSymbol("x", true); + any->identifier_->MapTo(x_sym); + ident_x->MapTo(x_sym); + auto value = Eval(any); + EXPECT_TRUE(value.ValueBool()); +} + +TEST_F(ExpressionEvaluatorTest, FunctionNoneNullElementInList2) { + AstStorage storage; + auto *ident_x = IDENT("x"); + auto *none = NONE("x", LIST(LITERAL(0), LITERAL(storage::PropertyValue())), + WHERE(EQ(ident_x, LITERAL(0)))); + const auto x_sym = symbol_table.CreateSymbol("x", true); + none->identifier_->MapTo(x_sym); + ident_x->MapTo(x_sym); + auto value = Eval(none); + EXPECT_FALSE(value.ValueBool()); +} + +TEST_F(ExpressionEvaluatorTest, FunctionNoneWhereWrongType) { + AstStorage storage; + auto *none = NONE("x", LIST(LITERAL(1)), WHERE(LITERAL(2))); + const auto x_sym = symbol_table.CreateSymbol("x", true); + none->identifier_->MapTo(x_sym); + EXPECT_THROW(Eval(none), QueryRuntimeException); +} + TEST_F(ExpressionEvaluatorTest, FunctionReduce) { AstStorage storage; auto *ident_sum = IDENT("sum");