From 6c22caa80ecf0269cd54c50441f54cc952eb1a4c Mon Sep 17 00:00:00 2001 From: florijan Date: Fri, 4 Aug 2017 09:44:51 +0200 Subject: [PATCH] Query::Plan::Expand with preceeding optional bug fix Summary: This diff contains a bug fix for the expansion operators that are currently on dev. More importantly, it proposes end-to-end testing for edge-cases for which it's a pain to write single-phase tests. In my opinion this is OK, you're all reviewers so you can comment. The test relies on left-to-right query execution. We need this guarantee in tests like this. I propose renaming "RuleBasedPlanner" to "LeftToRightPlanner" to make this explicit. As Teon is not here at the moment, will make this a task/discussion. Reviewers: buda, mislav.bradac, teon.banek, lion Reviewed By: mislav.bradac Subscribers: mferencevic, pullbot Differential Revision: https://phabricator.memgraph.io/D626 --- src/query/plan/operator.cpp | 122 ++++++++++++++------------- tests/unit/query_plan_edge_cases.cpp | 61 ++++++++++++++ 2 files changed, 126 insertions(+), 57 deletions(-) create mode 100644 tests/unit/query_plan_edge_cases.cpp diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 709172daf..e2e7e1499 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -478,35 +478,39 @@ void SwitchAccessor(TAccessor &accessor, GraphView graph_view) { bool Expand::ExpandCursor::InitEdges(Frame &frame, const SymbolTable &symbol_table) { - if (!input_cursor_->Pull(frame, symbol_table)) return false; + // Input Vertex could be null if it is created by a failed optional match. In + // those cases we skip that input pull and continue with the next. + while (true) { + if (!input_cursor_->Pull(frame, symbol_table)) return false; + TypedValue &vertex_value = frame[self_.input_symbol_]; - TypedValue &vertex_value = frame[self_.input_symbol_]; - // Vertex could be null if it is created by a failed optional match, in such a - // case we should stop expanding. - if (vertex_value.IsNull()) return false; - ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex); - auto &vertex = vertex_value.Value(); - SwitchAccessor(vertex, self_.graph_view_); + // Null check due to possible failed optional match. + if (vertex_value.IsNull()) continue; - auto direction = self_.direction_; - if (direction == EdgeAtom::Direction::IN || - direction == EdgeAtom::Direction::BOTH) { - in_edges_ = std::make_unique(vertex.in()); - in_edges_it_ = std::make_unique(in_edges_->begin()); + ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex); + auto &vertex = vertex_value.Value(); + SwitchAccessor(vertex, self_.graph_view_); + + auto direction = self_.direction_; + if (direction == EdgeAtom::Direction::IN || + direction == EdgeAtom::Direction::BOTH) { + in_edges_ = std::make_unique(vertex.in()); + in_edges_it_ = std::make_unique(in_edges_->begin()); + } + + if (direction == EdgeAtom::Direction::OUT || + direction == EdgeAtom::Direction::BOTH) { + out_edges_ = std::make_unique(vertex.out()); + out_edges_it_ = std::make_unique(out_edges_->begin()); + } + + // TODO add support for Front and Back expansion (when QueryPlanner + // will need it). For now only Back expansion (left to right) is + // supported + // TODO add support for named paths + + return true; } - - if (direction == EdgeAtom::Direction::OUT || - direction == EdgeAtom::Direction::BOTH) { - out_edges_ = std::make_unique(vertex.out()); - out_edges_it_ = std::make_unique(out_edges_->begin()); - } - - // TODO add support for Front and Back expansion (when QueryPlanner - // will need it). For now only Back expansion (left to right) is - // supported - // TODO add support for named paths - - return true; } bool Expand::ExpandCursor::PullNode(const EdgeAccessor &new_edge, @@ -666,41 +670,45 @@ class ExpandVariableCursor : public Cursor { * is exhausted. */ bool PullInput(Frame &frame, const SymbolTable &symbol_table) { - if (!input_cursor_->Pull(frame, symbol_table)) return false; + // Input Vertex could be null if it is created by a failed optional match. + // In those cases we skip that input pull and continue with the next. + while (true) { + if (!input_cursor_->Pull(frame, symbol_table)) return false; + TypedValue &vertex_value = frame[self_.input_symbol_]; - TypedValue &vertex_value = frame[self_.input_symbol_]; - // Vertex could be null if it is created by a failed optional match, in - // such a case we should stop expanding. - if (vertex_value.IsNull()) return false; - ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex); - auto &vertex = vertex_value.Value(); - SwitchAccessor(vertex, self_.graph_view_); + // Null check due to possible failed optional match. + if (vertex_value.IsNull()) continue; - // evaluate the upper and lower bounds - ExpressionEvaluator evaluator(frame, symbol_table, db_); - auto calc_bound = [this, &evaluator](auto &bound) { - auto value = EvaluateInt(evaluator, bound, "Variable expansion bound"); - if (value < 0) - throw QueryRuntimeException( - "Variable expansion bound must be positive or zero"); - return value; - }; - - lower_bound_ = self_.lower_bound_ ? calc_bound(self_.lower_bound_) : 1; - upper_bound_ = self_.upper_bound_ ? calc_bound(self_.upper_bound_) - : std::numeric_limits::max(); - - if (upper_bound_ > 0) { + ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex); + auto &vertex = vertex_value.Value(); SwitchAccessor(vertex, self_.graph_view_); - edges_.emplace_back(ExpandFromVertex(vertex, self_.direction_)); - edges_it_.emplace_back(edges_.back().begin()); + + // Evaluate the upper and lower bounds. + ExpressionEvaluator evaluator(frame, symbol_table, db_); + auto calc_bound = [this, &evaluator](auto &bound) { + auto value = EvaluateInt(evaluator, bound, "Variable expansion bound"); + if (value < 0) + throw QueryRuntimeException( + "Variable expansion bound must be positive or zero"); + return value; + }; + + lower_bound_ = self_.lower_bound_ ? calc_bound(self_.lower_bound_) : 1; + upper_bound_ = self_.upper_bound_ ? calc_bound(self_.upper_bound_) + : std::numeric_limits::max(); + + if (upper_bound_ > 0) { + SwitchAccessor(vertex, self_.graph_view_); + edges_.emplace_back(ExpandFromVertex(vertex, self_.direction_)); + edges_it_.emplace_back(edges_.back().begin()); + } + + // reset the frame value to an empty edge list + if (!self_.existing_edge_) + frame[self_.edge_symbol_] = std::vector(); + + return true; } - - // reset the frame value to an empty edge list - if (!self_.existing_edge_) - frame[self_.edge_symbol_] = std::vector(); - - return true; } /** diff --git a/tests/unit/query_plan_edge_cases.cpp b/tests/unit/query_plan_edge_cases.cpp new file mode 100644 index 000000000..b11754fd6 --- /dev/null +++ b/tests/unit/query_plan_edge_cases.cpp @@ -0,0 +1,61 @@ +// tests in this suite deal with edge cases in logical operator behavior +// that's not easily testable with single-phase testing. instead, for +// easy testing and latter readability they are tested end-to-end. + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include "communication/result_stream_faker.hpp" +#include "database/dbms.hpp" +#include "query/interpreter.hpp" + +class QueryExecution : public testing::Test { + protected: + Dbms dbms_; + std::unique_ptr db_ = dbms_.active(); + + /** Commits the current transaction and refreshes the db_ + * variable to hold a new accessor with a new transaction */ + void Commit() { + db_->commit(); + auto next_db = dbms_.active(); + db_.swap(next_db); + } + + /** Executes the query and returns the results. + * Does NOT commit the transaction */ + auto Execute(const std::string &query) { + ResultStreamFaker results; + query::Interpreter().Interpret(query, *db_, results, {}); + return results.GetResults(); + } +}; + +TEST_F(QueryExecution, MissingOptionalIntoExpand) { + // validating bug where expanding from Null (due to a preceeding optional + // match) exhausts the expansion cursor, even if it's input is still not + // exhausted + Execute( + "CREATE (a:Person {id: 1}), (b:Person " + "{id:2})-[:Has]->(:Dog)-[:Likes]->(:Food )"); + Commit(); + ASSERT_EQ(Execute("MATCH (n) RETURN n").size(), 4); + + auto Exec = [this](bool desc, bool variable) { + // this test depends on left-to-right query planning + FLAGS_query_cost_planner = false; + return Execute(std::string("MATCH (p:Person) WITH p ORDER BY p.id ") + + (desc ? "DESC " : "") + + "OPTIONAL MATCH (p)-->(d:Dog) WITH p, d " + "MATCH (d)-" + (variable ? "[*1]" : "") + "->(f:Food) " + "RETURN p, d, f") + .size(); + }; + + EXPECT_EQ(Exec(false, false), 1); + EXPECT_EQ(Exec(true, false), 1); + EXPECT_EQ(Exec(false, true), 1); + EXPECT_EQ(Exec(true, true), 1); + + // TODO test/fix ExpandBreadthFirst once it's operator lands +}