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

@@ -478,12 +478,15 @@ void SwitchAccessor(TAccessor &accessor, GraphView graph_view) {
 
 bool Expand::ExpandCursor::InitEdges(Frame &frame,
                                      const SymbolTable &symbol_table) {
+  // 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_];
-  // 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;
+
+    // Null check due to possible failed optional match.
+    if (vertex_value.IsNull()) continue;
+
     ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex);
     auto &vertex = vertex_value.Value<VertexAccessor>();
     SwitchAccessor(vertex, self_.graph_view_);
@@ -507,6 +510,7 @@ bool Expand::ExpandCursor::InitEdges(Frame &frame,
     // TODO add support for named paths
 
     return true;
+  }
 }
 
 bool Expand::ExpandCursor::PullNode(const EdgeAccessor &new_edge,
@@ -666,17 +670,20 @@ class ExpandVariableCursor : public Cursor {
    * is exhausted.
    */
   bool PullInput(Frame &frame, const SymbolTable &symbol_table) {
+    // 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_];
-    // 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;
+
+      // Null check due to possible failed optional match.
+      if (vertex_value.IsNull()) continue;
+
       ExpectType(self_.input_symbol_, vertex_value, TypedValue::Type::Vertex);
       auto &vertex = vertex_value.Value<VertexAccessor>();
       SwitchAccessor(vertex, self_.graph_view_);
 
-    // evaluate the upper and lower bounds
+      // 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");
@@ -702,6 +709,7 @@ class ExpandVariableCursor : public Cursor {
 
       return true;
     }
+  }
 
   /**
    * Enum that indicates what happened during attempted edge

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<GraphDbAccessor> 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
+}