From 9e157eb495758c8ba2b9de2bf80cb774cd09527c Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Wed, 26 Jul 2017 11:09:43 +0200 Subject: [PATCH] Generate All to filter variable length paths Summary: Use All to filter variable length paths. Test filtered variable length paths in qa Reviewers: florijan, mislav.bradac Reviewed By: mislav.bradac Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D592 --- src/query/plan/planner.hpp | 7 +- src/query/plan/rule_based_planner.cpp | 67 +++++++++++++------ .../tests/memgraph_V1/features/match.feature | 27 ++++++++ tests/unit/query_planner.cpp | 15 +++++ 4 files changed, 91 insertions(+), 25 deletions(-) diff --git a/src/query/plan/planner.hpp b/src/query/plan/planner.hpp index 08318917d..de46cbc5a 100644 --- a/src/query/plan/planner.hpp +++ b/src/query/plan/planner.hpp @@ -60,7 +60,7 @@ class Filters { /// for found labels, properties and edge types. The generated expressions are /// stored in @c all_filters. Also, @c label_filters and @c property_filters /// are populated. - void CollectPatternFilters(Pattern &, const SymbolTable &, AstTreeStorage &); + void CollectPatternFilters(Pattern &, SymbolTable &, AstTreeStorage &); /// Collects filtering information from a where expression. /// /// Takes the where expression and stores it in @c all_filters, then analyzes @@ -203,8 +203,9 @@ class VariableStartPlanner { /// /// This function will normalize patterns inside @c Match and @c Merge clauses /// and do some other preprocessing in order to generate multiple @c QueryPart -/// structures. -std::vector CollectQueryParts(const SymbolTable &, AstTreeStorage &); +/// structures. @c AstTreeStorage and @c SymbolTable may be used to create new +/// AST nodes. +std::vector CollectQueryParts(SymbolTable &, AstTreeStorage &); /// @brief Generates the LogicalOperator tree and returns the resulting plan. /// diff --git a/src/query/plan/rule_based_planner.cpp b/src/query/plan/rule_based_planner.cpp index ede9f459e..3d5658796 100644 --- a/src/query/plan/rule_based_planner.cpp +++ b/src/query/plan/rule_based_planner.cpp @@ -62,7 +62,7 @@ auto ReducePattern( return last_res; } -void ForeachPattern( +void ForEachPattern( Pattern &pattern, std::function base, std::function collect) { debug_assert(!pattern.atoms_.empty(), "Missing atoms in pattern"); @@ -643,7 +643,7 @@ std::vector NormalizePatterns( debug_assert(node, "First pattern atom is not a node"); expansions.emplace_back(Expansion{node}); } else { - ForeachPattern(*pattern, ignore_node, collect_expansion); + ForEachPattern(*pattern, ignore_node, collect_expansion); } } return expansions; @@ -655,7 +655,7 @@ std::vector NormalizePatterns( // will lift them out of a pattern and generate new expressions (just like they // were in a Where clause). void AddMatching(const std::vector &patterns, Where *where, - const SymbolTable &symbol_table, AstTreeStorage &storage, + SymbolTable &symbol_table, AstTreeStorage &storage, Matching &matching) { auto expansions = NormalizePatterns(symbol_table, patterns); std::unordered_set edge_symbols; @@ -676,7 +676,7 @@ void AddMatching(const std::vector &patterns, Where *where, matching.filters.CollectWhereFilter(*where, symbol_table); } } -void AddMatching(const Match &match, const SymbolTable &symbol_table, +void AddMatching(const Match &match, SymbolTable &symbol_table, AstTreeStorage &storage, Matching &matching) { return AddMatching(match.patterns_, match.where_, symbol_table, storage, matching); @@ -1015,25 +1015,39 @@ void Filters::AnalyzeFilter(Expression *expr, const SymbolTable &symbol_table) { // as `expr1 < n.prop AND n.prop < expr2`. } -void Filters::CollectPatternFilters(Pattern &pattern, - const SymbolTable &symbol_table, +void Filters::CollectPatternFilters(Pattern &pattern, SymbolTable &symbol_table, AstTreeStorage &storage) { UsedSymbolsCollector collector(symbol_table); - auto add_properties_filter = [&](auto *atom) { + auto add_properties_filter = [&](auto *atom, bool is_variable_path = false) { const auto &symbol = symbol_table.at(*atom->identifier_); for (auto &prop_pair : atom->properties_) { collector.symbols_.clear(); prop_pair.second->Accept(collector); - // Store a PropertyFilter on the value of the property. - property_filters_[symbol][prop_pair.first].emplace_back( - PropertyFilter{collector.symbols_, prop_pair.second}); + auto *identifier = atom->identifier_; + if (is_variable_path) { + // Create a new identifier and a symbol which will be filled in All. + identifier = identifier->Clone(storage); + symbol_table[*identifier] = + symbol_table.CreateSymbol(identifier->name_, false); + } else { + // Store a PropertyFilter on the value of the property. + property_filters_[symbol][prop_pair.first].emplace_back( + PropertyFilter{collector.symbols_, prop_pair.second}); + } // Create an equality expression and store it in all_filters_. auto *property_lookup = - storage.Create(atom->identifier_, prop_pair.first); + storage.Create(identifier, prop_pair.first); auto *prop_equal = storage.Create(property_lookup, prop_pair.second); collector.symbols_.insert(symbol); // PropertyLookup uses the symbol. - all_filters_.emplace_back(prop_equal, collector.symbols_); + if (is_variable_path) { + all_filters_.emplace_back( + storage.Create(identifier, atom->identifier_, + storage.Create(prop_equal)), + collector.symbols_); + } else { + all_filters_.emplace_back(prop_equal, collector.symbols_); + } } }; auto add_node_filter = [&](NodeAtom *node) { @@ -1052,19 +1066,28 @@ void Filters::CollectPatternFilters(Pattern &pattern, auto add_expand_filter = [&](NodeAtom *prev_node, EdgeAtom *edge, NodeAtom *node) { const auto &edge_symbol = symbol_table.at(*edge->identifier_); - if (edge->has_range_ && - (!edge->edge_types_.empty() || !edge->properties_.empty())) { - throw utils::NotYetImplemented("filtering variable length paths"); - } if (!edge->edge_types_.empty()) { - all_filters_.emplace_back( - storage.Create(edge->identifier_, edge->edge_types_), - std::unordered_set{edge_symbol}); + if (edge->has_range_) { + // We need a new identifier and symbol for All. + auto *identifier = edge->identifier_->Clone(storage); + symbol_table[*identifier] = + symbol_table.CreateSymbol(identifier->name_, false); + auto *edge_type_test = + storage.Create(identifier, edge->edge_types_); + all_filters_.emplace_back( + storage.Create(identifier, edge->identifier_, + storage.Create(edge_type_test)), + std::unordered_set{edge_symbol}); + } else { + all_filters_.emplace_back( + storage.Create(edge->identifier_, edge->edge_types_), + std::unordered_set{edge_symbol}); + } } - add_properties_filter(edge); + add_properties_filter(edge, edge->has_range_); add_node_filter(node); }; - ForeachPattern(pattern, add_node_filter, add_expand_filter); + ForEachPattern(pattern, add_node_filter, add_expand_filter); } // Adds the where filter expression to all filters and collects additional @@ -1079,7 +1102,7 @@ void Filters::CollectWhereFilter(Where &where, // Converts a Query to multiple QueryParts. In the process new Ast nodes may be // created, e.g. filter expressions. -std::vector CollectQueryParts(const SymbolTable &symbol_table, +std::vector CollectQueryParts(SymbolTable &symbol_table, AstTreeStorage &storage) { auto query = storage.query(); std::vector query_parts(1); diff --git a/tests/qa/tck_engine/tests/memgraph_V1/features/match.feature b/tests/qa/tck_engine/tests/memgraph_V1/features/match.feature index 8bd497802..502288597 100644 --- a/tests/qa/tck_engine/tests/memgraph_V1/features/match.feature +++ b/tests/qa/tck_engine/tests/memgraph_V1/features/match.feature @@ -457,3 +457,30 @@ Feature: Match | 2 | 3 | | 3 | 3 | + Scenario: Test match filtered edge type variable path + Given an empty graph + And having executed: + """ + CREATE ({a: 1}) -[:r1]-> ({a:2}) -[:r2]-> ({a:3}) + """ + When executing query: + """ + MATCH (n) -[:r1*]-> (m) RETURN n.a, m.a + """ + Then the result should be: + | n.a | m.a | + | 1 | 2 | + + Scenario: Test match filtered properties variable path + Given an empty graph + And having executed: + """ + CREATE ({a: 1}) -[:r {p1: 1, p2: 2}]-> ({a:2}) -[:r {p1: 1, p2: 3}]-> ({a:3}) + """ + When executing query: + """ + MATCH (n) -[*{p1: 1, p2:2}]-> (m) RETURN n.a, m.a + """ + Then the result should be: + | n.a | m.a | + | 1 | 2 | diff --git a/tests/unit/query_planner.cpp b/tests/unit/query_planner.cpp index 61439f53c..af37fa227 100644 --- a/tests/unit/query_planner.cpp +++ b/tests/unit/query_planner.cpp @@ -1236,4 +1236,19 @@ TEST(TestLogicalPlanner, MatchExpandVariableNoBounds) { CheckPlan(storage, ExpectScanAll(), ExpectExpandVariable(), ExpectProduce()); } +TEST(TestLogicalPlanner, MatchExpandVariableFiltered) { + // Test MATCH (n) -[r :type * {prop: 42}]-> (m) RETURN r + Dbms dbms; + auto dba = dbms.active(); + auto type = dba->edge_type("type"); + auto prop = dba->property("prop"); + AstTreeStorage storage; + auto edge = EDGE("r", type); + edge->has_range_ = true; + edge->properties_[prop] = LITERAL(42); + QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("r")); + CheckPlan(storage, ExpectScanAll(), ExpectExpandVariable(), ExpectFilter(), + ExpectProduce()); +} + } // namespace