Resolving TODOs in the operator implementations

Summary:
Three TODOs resolved.

1. around line 897 - we currently don't support expansion into existing variable length edges (there is a TODO in symbol_generator.cpp:213), so this should not be done at the moment.

2. around line 1025 - This TODO was on review and nobody commented, so I'm removing it. Should have done that when the diff landed.

3. around line 1560 - This does not seem possible. Edge-uniqueness checks happen within a single `[OPTIONAL ] MATCH`. If it is OPTIONAL (the case interesting here), then the uniqueness check also gets planned under the optional branch. So, if an optional fails, the uniqueness check will get skipped, as opposed to getting executed over a Null. I added an edge-case test to verify this (and checked with the planner test).

Reviewers: buda, teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D807

src/query/plan/operator.cpp

@@ -887,7 +887,6 @@ class ExpandVariableCursor : public Cursor {
       if (edges_.empty()) return false;
 
       // we use this a lot
-      // TODO handle optional + existing edge
       std::vector<TypedValue> &edges_on_frame =
           frame[self_.edge_symbol_].Value<std::vector<TypedValue>>();
 
@@ -1016,7 +1015,6 @@ bool ExpandBreadthFirst::Cursor::Pull(Frame &frame, Context &context) {
     TypedValue result = self_.where_->Accept(evaluator);
     switch (result.type()) {
       case TypedValue::Type::Null:
-        // TODO review: is treating Null as false desired?
         return;
       case TypedValue::Type::Bool:
         if (!result.Value<bool>()) return;
@@ -1630,9 +1628,8 @@ bool ExpandUniquenessFilter<TAccessor>::ExpandUniquenessFilterCursor::Pull(
     for (const auto &previous_symbol : self_.previous_symbols_) {
       TypedValue &previous_value = frame[previous_symbol];
       // This shouldn't raise a TypedValueException, because the planner makes
-      // sure these are all of the expected type. In case they are not, an
-      // error should be raised long before this code is executed.
-      // TODO handle possible null due to optional match
+      // sure these are all of the expected type. In case they are not, an error
+      // should be raised long before this code is executed.
       if (ContainsSame<TAccessor>(previous_value, expand_value)) return false;
     }
     return true;

tests/unit/query_plan_edge_cases.cpp

@@ -41,23 +41,38 @@ TEST_F(QueryExecution, MissingOptionalIntoExpand) {
   Commit();
   ASSERT_EQ(Execute("MATCH (n) RETURN n").size(), 4);
 
-  auto Exec = [this](bool desc, bool variable) {
+  auto Exec = [this](bool desc, const std::string &edge_pattern) {
     // 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) "
+                   "MATCH (d)" + edge_pattern +
+                   "(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);
+  std::string expand = "-->";
+  std::string variable = "-[*1]->";
+  std::string bfs = "-bfs[](n, e | true, 1)->";
 
-  // TODO test/fix ExpandBreadthFirst once it's operator lands
+  EXPECT_EQ(Exec(false, expand), 1);
+  EXPECT_EQ(Exec(true, expand), 1);
+  EXPECT_EQ(Exec(false, variable), 1);
+  EXPECT_EQ(Exec(true, bfs), 1);
+  EXPECT_EQ(Exec(true, bfs), 1);
+}
+
+TEST_F(QueryExecution, EdgeUniquenessInOptional) {
+  // Validating that an edge uniqueness check can't fail when the edge is Null
+  // due to optonal match. Since edge-uniqueness only happens in one OPTIONAL
+  // MATCH, we only need to check that scenario.
+  Execute("CREATE (), ()-[:Type]->()");
+  Commit();
+  ASSERT_EQ(Execute("MATCH (n) RETURN n").size(), 3);
+  EXPECT_EQ(Execute("MATCH (n) OPTIONAL MATCH (n)-[r1]->(), (n)-[r2]->() "
+                    "RETURN n, r1, r2")
+                .size(),
+            3);
 }