From 4a602a445a4d3f425cde0243d593343573829638 Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Wed, 20 Sep 2017 15:31:23 +0200 Subject: [PATCH] Split MATCH ... WHERE filtering on AND Summary: Test planner splits MATCH ... WHERE Remove distinction between FilterAnd and AndOperator Reviewers: florijan, mislav.bradac Reviewed By: mislav.bradac Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D814 --- src/query/frontend/ast/ast.hpp | 21 ----------- src/query/frontend/ast/ast_visitor.hpp | 45 +++++++++++------------ src/query/interpret/eval.hpp | 3 +- src/query/plan/rule_based_planner.cpp | 37 +++++++++++++++---- src/query/plan/rule_based_planner.hpp | 7 ++-- tests/unit/query_expression_evaluator.cpp | 17 +++------ tests/unit/query_planner.cpp | 14 +++++++ 7 files changed, 75 insertions(+), 69 deletions(-) diff --git a/src/query/frontend/ast/ast.hpp b/src/query/frontend/ast/ast.hpp index b39a0b12d..2e6f98aab 100644 --- a/src/query/frontend/ast/ast.hpp +++ b/src/query/frontend/ast/ast.hpp @@ -192,27 +192,6 @@ class AndOperator : public BinaryOperator { using BinaryOperator::BinaryOperator; }; -// This is separate operator so that we can implement different short-circuiting -// semantics than regular AndOperator. At this point CypherMainVisitor shouldn't -// concern itself with this, and should constructor only AndOperator-s. This is -// used in query planner at the moment. -class FilterAndOperator : public BinaryOperator { - friend class AstTreeStorage; - - public: - DEFVISITABLE(TreeVisitor); - bool Accept(HierarchicalTreeVisitor &visitor) override { - if (visitor.PreVisit(*this)) { - expression1_->Accept(visitor) && expression2_->Accept(visitor); - } - return visitor.PostVisit(*this); - } - CLONE_BINARY_EXPRESSION; - - protected: - using BinaryOperator::BinaryOperator; -}; - class AdditionOperator : public BinaryOperator { friend class AstTreeStorage; diff --git a/src/query/frontend/ast/ast_visitor.hpp b/src/query/frontend/ast/ast_visitor.hpp index 7aa7404d9..c92ad9c66 100644 --- a/src/query/frontend/ast/ast_visitor.hpp +++ b/src/query/frontend/ast/ast_visitor.hpp @@ -29,7 +29,6 @@ class MapLiteral; class OrOperator; class XorOperator; class AndOperator; -class FilterAndOperator; class NotOperator; class AdditionOperator; class SubtractionOperator; @@ -61,16 +60,16 @@ class Unwind; class CreateIndex; using TreeCompositeVisitor = ::utils::CompositeVisitor< - Query, NamedExpression, OrOperator, XorOperator, AndOperator, - FilterAndOperator, NotOperator, AdditionOperator, SubtractionOperator, - MultiplicationOperator, DivisionOperator, ModOperator, NotEqualOperator, - EqualOperator, LessOperator, GreaterOperator, LessEqualOperator, - GreaterEqualOperator, InListOperator, ListMapIndexingOperator, - ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator, - IsNullOperator, ListLiteral, MapLiteral, PropertyLookup, LabelsTest, - EdgeTypeTest, Aggregation, Function, All, Create, Match, Return, With, - Pattern, NodeAtom, EdgeAtom, BreadthFirstAtom, Delete, Where, SetProperty, - SetProperties, SetLabels, RemoveProperty, RemoveLabels, Merge, Unwind>; + Query, NamedExpression, OrOperator, XorOperator, AndOperator, NotOperator, + AdditionOperator, SubtractionOperator, MultiplicationOperator, + DivisionOperator, ModOperator, NotEqualOperator, EqualOperator, + LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator, + InListOperator, ListMapIndexingOperator, ListSlicingOperator, IfOperator, + UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral, + MapLiteral, PropertyLookup, LabelsTest, EdgeTypeTest, Aggregation, Function, + All, Create, Match, Return, With, Pattern, NodeAtom, EdgeAtom, + BreadthFirstAtom, Delete, Where, SetProperty, SetProperties, SetLabels, + RemoveProperty, RemoveLabels, Merge, Unwind>; using TreeLeafVisitor = ::utils::LeafVisitor; @@ -78,24 +77,24 @@ using TreeLeafVisitor = ::utils::LeafVisitor using TreeVisitor = ::utils::Visitor< TResult, Query, NamedExpression, OrOperator, XorOperator, AndOperator, - FilterAndOperator, NotOperator, AdditionOperator, SubtractionOperator, - MultiplicationOperator, DivisionOperator, ModOperator, NotEqualOperator, - EqualOperator, LessOperator, GreaterOperator, LessEqualOperator, - GreaterEqualOperator, InListOperator, ListMapIndexingOperator, - ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator, - IsNullOperator, ListLiteral, MapLiteral, PropertyLookup, LabelsTest, - EdgeTypeTest, Aggregation, Function, All, ParameterLookup, Create, Match, - Return, With, Pattern, NodeAtom, EdgeAtom, BreadthFirstAtom, Delete, Where, - SetProperty, SetProperties, SetLabels, RemoveProperty, RemoveLabels, Merge, - Unwind, Identifier, PrimitiveLiteral, CreateIndex>; + NotOperator, AdditionOperator, SubtractionOperator, MultiplicationOperator, + DivisionOperator, ModOperator, NotEqualOperator, EqualOperator, + LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator, + InListOperator, ListMapIndexingOperator, ListSlicingOperator, IfOperator, + UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral, + MapLiteral, PropertyLookup, LabelsTest, EdgeTypeTest, Aggregation, Function, + All, ParameterLookup, Create, Match, Return, With, Pattern, NodeAtom, + EdgeAtom, BreadthFirstAtom, Delete, Where, SetProperty, SetProperties, + SetLabels, RemoveProperty, RemoveLabels, Merge, Unwind, Identifier, + PrimitiveLiteral, CreateIndex>; } // namespace query diff --git a/src/query/interpret/eval.hpp b/src/query/interpret/eval.hpp index c7487b5a5..b74fcd405 100644 --- a/src/query/interpret/eval.hpp +++ b/src/query/interpret/eval.hpp @@ -96,7 +96,6 @@ class ExpressionEvaluator : public TreeVisitor { BINARY_OPERATOR_VISITOR(OrOperator, ||, OR); BINARY_OPERATOR_VISITOR(XorOperator, ^, XOR); - BINARY_OPERATOR_VISITOR(AndOperator, &&, AND); BINARY_OPERATOR_VISITOR(AdditionOperator, +, +); BINARY_OPERATOR_VISITOR(SubtractionOperator, -, -); BINARY_OPERATOR_VISITOR(MultiplicationOperator, *, *); @@ -116,7 +115,7 @@ class ExpressionEvaluator : public TreeVisitor { #undef BINARY_OPERATOR_VISITOR #undef UNARY_OPERATOR_VISITOR - TypedValue Visit(FilterAndOperator &op) override { + TypedValue Visit(AndOperator &op) override { auto value1 = op.expression1_->Accept(*this); if (value1.IsNull() || !value1.Value()) { // If first expression is null or false, don't execute the second one. diff --git a/src/query/plan/rule_based_planner.cpp b/src/query/plan/rule_based_planner.cpp index 9de49c5c3..bd6dbfc89 100644 --- a/src/query/plan/rule_based_planner.cpp +++ b/src/query/plan/rule_based_planner.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "utils/algorithm.hpp" @@ -331,7 +332,6 @@ class ReturnBodyContext : public HierarchicalTreeVisitor { VISIT_BINARY_OPERATOR(OrOperator) VISIT_BINARY_OPERATOR(XorOperator) VISIT_BINARY_OPERATOR(AndOperator) - VISIT_BINARY_OPERATOR(FilterAndOperator) VISIT_BINARY_OPERATOR(AdditionOperator) VISIT_BINARY_OPERATOR(SubtractionOperator) VISIT_BINARY_OPERATOR(MultiplicationOperator) @@ -608,7 +608,7 @@ void AddMatching(const Match &match, SymbolTable &symbol_table, } // Iterates over `all_filters` joining them in one expression via -// `FilterAndOperator`. Filters which use unbound symbols are skipped, as well +// `AndOperator`. Filters which use unbound symbols are skipped, as well // as those that fail the `predicate` function. The function takes a single // argument, `FilterInfo`. All the joined filters are removed from // `all_filters`. @@ -622,8 +622,8 @@ Expression *ExtractFilters(const std::unordered_set &bound_symbols, filters_it != all_filters.end();) { if (HasBoundFilterSymbols(bound_symbols, *filters_it) && predicate(*filters_it)) { - filter_expr = impl::BoolJoin(storage, filter_expr, - filters_it->expression); + filter_expr = impl::BoolJoin(storage, filter_expr, + filters_it->expression); filters_it = all_filters.erase(filters_it); } else { filters_it++; @@ -631,6 +631,24 @@ Expression *ExtractFilters(const std::unordered_set &bound_symbols, } return filter_expr; } + +auto SplitExpressionOnAnd(Expression *expression) { + std::vector expressions; + std::stack pending_expressions; + pending_expressions.push(expression); + while (!pending_expressions.empty()) { + auto *current_expression = pending_expressions.top(); + pending_expressions.pop(); + if (auto *and_op = dynamic_cast(current_expression)) { + pending_expressions.push(and_op->expression1_); + pending_expressions.push(and_op->expression2_); + } else { + expressions.push_back(current_expression); + } + } + return expressions; +} + } // namespace namespace impl { @@ -995,10 +1013,13 @@ void Filters::CollectPatternFilters(Pattern &pattern, SymbolTable &symbol_table, // information for potential property and label indexing. void Filters::CollectWhereFilter(Where &where, const SymbolTable &symbol_table) { - UsedSymbolsCollector collector(symbol_table); - where.expression_->Accept(collector); - all_filters_.emplace_back(FilterInfo{where.expression_, collector.symbols_}); - AnalyzeFilter(where.expression_, symbol_table); + auto where_filters = SplitExpressionOnAnd(where.expression_); + for (const auto &filter : where_filters) { + UsedSymbolsCollector collector(symbol_table); + filter->Accept(collector); + all_filters_.emplace_back(FilterInfo{filter, collector.symbols_}); + AnalyzeFilter(filter, symbol_table); + } } // Converts a Query to multiple QueryParts. In the process new Ast nodes may be diff --git a/src/query/plan/rule_based_planner.hpp b/src/query/plan/rule_based_planner.hpp index 6d0023647..a9805a4c9 100644 --- a/src/query/plan/rule_based_planner.hpp +++ b/src/query/plan/rule_based_planner.hpp @@ -548,9 +548,10 @@ class RuleBasedPlanner { const auto &next_node_symbol = symbol_table.at(*bf_atom->next_node_identifier_); // Inline BFS edge filtering together with its filter expression. - auto *filter_expr = impl::BoolJoin( - storage, impl::ExtractMultiExpandFilter( - bound_symbols, node_symbol, all_filters, storage), + auto *filter_expr = impl::BoolJoin( + storage, + impl::ExtractMultiExpandFilter(bound_symbols, node_symbol, + all_filters, storage), bf_atom->filter_expression_); last_op = new ExpandBreadthFirst( node_symbol, edge_symbol, expansion.direction, edge_type, diff --git a/tests/unit/query_expression_evaluator.cpp b/tests/unit/query_expression_evaluator.cpp index 37815f167..3f2a6a5fa 100644 --- a/tests/unit/query_expression_evaluator.cpp +++ b/tests/unit/query_expression_evaluator.cpp @@ -101,25 +101,18 @@ TEST(ExpressionEvaluator, AndOperator) { ASSERT_EQ(val2.Value(), false); } -TEST(ExpressionEvaluator, FilterAndOperator) { +TEST(ExpressionEvaluator, AndOperatorShortCircuit) { AstTreeStorage storage; NoContextExpressionEvaluator eval; { - auto *op = storage.Create( - storage.Create(true), - storage.Create(true)); - auto value = op->Accept(eval.eval); - EXPECT_EQ(value.Value(), true); - } - { - auto *op = storage.Create( - storage.Create(false), - storage.Create(5)); + auto *op = + storage.Create(storage.Create(false), + storage.Create(5)); auto value = op->Accept(eval.eval); EXPECT_EQ(value.Value(), false); } { - auto *op = storage.Create( + auto *op = storage.Create( storage.Create(TypedValue::Null), storage.Create(5)); auto value = op->Accept(eval.eval); diff --git a/tests/unit/query_planner.cpp b/tests/unit/query_planner.cpp index 411f37291..9b33d08d3 100644 --- a/tests/unit/query_planner.cpp +++ b/tests/unit/query_planner.cpp @@ -1399,4 +1399,18 @@ TEST(TestLogicalPlanner, MatchScanToExpand) { ExpectFilter(), ExpectProduce()); } +TEST(TestLogicalPlanner, MatchWhereAndSplit) { + // Test MATCH (n) -[r]- (m) WHERE n.prop AND r.prop RETURN m + Dbms dbms; + auto dba = dbms.active(); + auto prop = PROPERTY_PAIR("prop"); + AstTreeStorage storage; + QUERY(MATCH(PATTERN(NODE("n"), EDGE("r"), NODE("m"))), + WHERE(AND(PROPERTY_LOOKUP("n", prop), PROPERTY_LOOKUP("r", prop))), + RETURN("m")); + // We expect `n.prop` filter right after scanning `n`. + CheckPlan(storage, ExpectScanAll(), ExpectFilter(), ExpectExpand(), + ExpectFilter(), ExpectProduce()); +} + } // namespace