From 9edc472eaf19144aec63e44b439c10b620c7427e Mon Sep 17 00:00:00 2001
From: florijan <florijan@memgraph.io>
Date: Mon, 18 Sep 2017 14:30:07 +0200
Subject: [PATCH] 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          |  7 ++----
 tests/unit/query_plan_edge_cases.cpp | 33 ++++++++++++++++++++--------
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp
index 41ca4779d..e6648be9e 100644
--- a/src/query/plan/operator.cpp
+++ b/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;
diff --git a/tests/unit/query_plan_edge_cases.cpp b/tests/unit/query_plan_edge_cases.cpp
index fe0dc4da6..97a202c65 100644
--- a/tests/unit/query_plan_edge_cases.cpp
+++ b/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);
 }