fix(optimizer): drop the rows where MATCH re-binds a node that OPTIONAL MATCH left NULL

src/include/optimizer/remove_unnecessary_join_optimizer.h

@@ -15,7 +15,8 @@ namespace optimizer {
  *   E(e) S(b)
  *    |
  *   S(a)
- * This optimizer prunes such redundant joins.
+ * This optimizer prunes such redundant joins. A join whose key an OPTIONAL MATCH can leave
+ * NULL is not redundant: it drops the rows with a NULL key, so it stays.
  */
 class RemoveUnnecessaryJoinOptimizer : public LogicalOperatorVisitor {
 public:

src/include/planner/operator/logical_hash_join.h

@@ -37,7 +37,7 @@ class LBUG_API LogicalHashJoin final : public LogicalOperator {
     static binder::expression_vector getJoinNodeIDs(
         const std::vector<join_condition_t>& joinConditions);
 
-    std::vector<join_condition_t> getJoinConditions() const { return joinConditions; }
+    const std::vector<join_condition_t>& getJoinConditions() const { return joinConditions; }
     common::JoinType getJoinType() const { return joinType; }
     bool hasMark() const { return mark != nullptr; }
     std::shared_ptr<binder::Expression> getMark() const { return mark; }

src/optimizer/remove_unnecessary_join_optimizer.cpp

@@ -1,8 +1,11 @@
 #include "optimizer/remove_unnecessary_join_optimizer.h"
 
+#include "planner/operator/logical_accumulate.h"
+#include "planner/operator/logical_cross_product.h"
 #include "planner/operator/logical_hash_join.h"
 #include "planner/operator/scan/logical_scan_node_table.h"
 
+using namespace lbug::binder;
 using namespace lbug::common;
 using namespace lbug::planner;
 
@@ -24,6 +27,84 @@ std::shared_ptr<LogicalOperator> RemoveUnnecessaryJoinOptimizer::visitOperator(
     return result;
 }
 
+// No default labels in these two: -Wswitch flags a new type, which counts as NULL-producing
+// until classified.
+static bool nullsBuildOnlyColumns(JoinType joinType) {
+    switch (joinType) {
+    case JoinType::INNER:
+        return false;
+    case JoinType::MARK:
+    case JoinType::COUNT:
+        // These output only their probe side, so no build column reaches the parent.
+        return false;
+    case JoinType::LEFT:
+        return true;
+    }
+    return true;
+}
+
+static bool isOptional(AccumulateType accumulateType) {
+    switch (accumulateType) {
+    case AccumulateType::REGULAR:
+        return false;
+    case AccumulateType::OPTIONAL_:
+        return true;
+    }
+    return true;
+}
+
+// An OPTIONAL MATCH that finds nothing binds its variables to NULL. They come from the build
+// side of a LEFT join or an optional cross product, or from under an optional accumulate.
+// MERGE's existence check also plans an OPTIONAL MATCH, marked; MERGE creates what it does
+// not find, so those leave no NULLs.
+static bool canBeNull(const LogicalOperator& op, const Expression& key) {
+    switch (op.getOperatorType()) {
+    case LogicalOperatorType::HASH_JOIN: {
+        const auto& join = op.constCast<LogicalHashJoin>();
+        if (nullsBuildOnlyColumns(join.getJoinType()) && !join.hasMark() &&
+            !op.getChild(0)->getSchema()->isExpressionInScope(key) &&
+            op.getChild(1)->getSchema()->isExpressionInScope(key)) {
+            return true;
+        }
+    } break;
+    case LogicalOperatorType::CROSS_PRODUCT: {
+        const auto& crossProduct = op.constCast<LogicalCrossProduct>();
+        if (isOptional(crossProduct.getAccumulateType()) && !crossProduct.hasMark() &&
+            op.getChild(1)->getSchema()->isExpressionInScope(key)) {
+            return true;
+        }
+    } break;
+    case LogicalOperatorType::ACCUMULATE: {
+        const auto& accumulate = op.constCast<LogicalAccumulate>();
+        if (isOptional(accumulate.getAccumulateType()) && !accumulate.hasMark() &&
+            op.getChild(0)->getSchema()->isExpressionInScope(key)) {
+            return true;
+        }
+    } break;
+    default:
+        // Other operators pass a key through or bind it non-NULL (scan, extend, MERGE).
+        break;
+    }
+    for (auto i = 0u; i < op.getNumChildren(); ++i) {
+        if (canBeNull(*op.getChild(i), key)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+// Joining with a full node scan drops the rows whose key is NULL, so the join is only
+// redundant when the kept side's keys are never NULL.
+static bool keptSideKeysCanBeNull(const LogicalHashJoin& hashJoin, uint32_t keptChildIdx) {
+    for (auto& [probeKey, buildKey] : hashJoin.getJoinConditions()) {
+        const auto& key = keptChildIdx == 0 ? *probeKey : *buildKey;
+        if (canBeNull(*hashJoin.getChild(keptChildIdx), key)) {
+            return true;
+        }
+    }
+    return false;
+}
+
 std::shared_ptr<LogicalOperator> RemoveUnnecessaryJoinOptimizer::visitHashJoinReplace(
     std::shared_ptr<LogicalOperator> op) {
     auto hashJoin = (LogicalHashJoin*)op.get();
@@ -45,14 +126,14 @@ std::shared_ptr<LogicalOperator> RemoveUnnecessaryJoinOptimizer::visitHashJoinRe
     // keep this condition tight.
     if (op->getChild(1)->getOperatorType() == LogicalOperatorType::SCAN_NODE_TABLE) {
         const auto scanNode = dynamic_cast_checked<LogicalScanNodeTable*>(op->getChild(1).get());
-        if (scanNode->getProperties().empty()) {
+        if (scanNode->getProperties().empty() && !keptSideKeysCanBeNull(*hashJoin, 0)) {
             // Build side is trivial. Prune build side.
             return op->getChild(0);
         }
     }
     if (op->getChild(0)->getOperatorType() == LogicalOperatorType::SCAN_NODE_TABLE) {
         const auto scanNode = dynamic_cast_checked<LogicalScanNodeTable*>(op->getChild(0).get());
-        if (scanNode->getProperties().empty()) {
+        if (scanNode->getProperties().empty() && !keptSideKeysCanBeNull(*hashJoin, 1)) {
             // Probe side is trivial. Prune probe side.
             return op->getChild(1);
         }

test/optimizer/optimizer_test.cpp

@@ -351,6 +351,42 @@ TEST_F(OptimizerTest, RemoveUnnecessaryJoinTest) {
     ASSERT_STREQ(getEncodedPlan(q1).c_str(), "E(b)S(a)");
 }
 
+TEST_F(OptimizerTest, RemoveUnnecessaryJoinNullableKeyTest) {
+    // b can be NULL after each OPTIONAL MATCH form, so the join with S(b) stays.
+    auto q1 = "MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) "
+              "WITH a, b MATCH (b) RETURN a.ID;";
+    ASSERT_STREQ(getEncodedPlan(q1).c_str(),
+        "HJ(b._ID){HJ(a._ID){S(a)}{HJ(a._ID){}{E(b)S(a)}}}{S(b)}");
+    auto q2 = "MATCH (a:person) WHERE a.ID < 3 OPTIONAL MATCH (b:organisation) "
+              "WHERE b.score > 1000 WITH a, b MATCH (b) RETURN a.ID;";
+    ASSERT_STREQ(getEncodedPlan(q2).c_str(), "HJ(b._ID){CP(){Filter()S(a)}{Filter()S(b)}}{S(b)}");
+    auto q3 = "OPTIONAL MATCH (b:organisation) WHERE b.score > 1000 WITH b MATCH (b) RETURN b.ID;";
+    ASSERT_STREQ(getEncodedPlan(q3).c_str(), "HJ(b._ID){Filter()S(b)}{S(b)}");
+    // a is the LEFT join's probe key and MERGE creates b, so neither is NULL: the join goes.
+    auto q4 = "MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) "
+              "WITH a, b MATCH (a) RETURN b.ID;";
+    ASSERT_STREQ(getEncodedPlan(q4).c_str(),
+        "HJ(a._ID){S(a)}{HJ(a._ID){}{HJ(b._ID){S(b)}{E(b)S(a)}}}");
+    auto q5 = "MATCH (a:person) WHERE a.ID = 0 "
+              "MERGE (a)-[:studyAt]->(b:organisation {ID: 100}) WITH a, b MATCH (b) RETURN a.ID;";
+    ASSERT_STREQ(getEncodedPlan(q5).c_str(),
+        "Filter()HJ(a._ID){IndexScan(a)}{HJ(b._ID){IndexScan(b)}{E(b)S(a)}}");
+    auto q6 = "MATCH (a:person) WHERE a.ID = 0 MERGE (b:organisation {ID: 100}) "
+              "WITH a, b MATCH (b) RETURN a.ID;";
+    ASSERT_STREQ(getEncodedPlan(q6).c_str(), "Filter()CP(){IndexScan(a)}{IndexScan(b)}");
+    auto q7 = "MERGE (b:organisation {ID: 100}) WITH b MATCH (b) RETURN b.ID;";
+    ASSERT_STREQ(getEncodedPlan(q7).c_str(), "Filter()IndexScan(b)");
+    // After an update the scan is the probe side.
+    auto q8 = "MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) "
+              "SET a.age = a.age WITH a, b MATCH (b) RETURN a.ID;";
+    ASSERT_STREQ(getEncodedPlan(q8).c_str(),
+        "HJ(b._ID){S(b)}{HJ(a._ID){S(a)}{HJ(a._ID){}{E(b)S(a)}}}");
+    auto q9 = "MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) "
+              "SET a.age = a.age WITH a, b MATCH (a) RETURN b.ID;";
+    ASSERT_STREQ(getEncodedPlan(q9).c_str(),
+        "HJ(a._ID){S(a)}{HJ(a._ID){}{HJ(b._ID){S(b)}{E(b)S(a)}}}");
+}
+
 TEST_F(OptimizerTest, MergeConsecutiveMatch) {
     auto q1 = "MATCH (a:person) MATCH (b:person) WHERE b.ID=0 MATCH (a)-[]->(b) "
               "RETURN COUNT(*);";

test/test_files/optional_match/optional_match.test

@@ -118,3 +118,64 @@ Hubert Blaine Wolfeschlegelsteinhausenbergerdorff|
 -STATEMENT MATCH (a:person) OPTIONAL MATCH (a)-[:knows]->(b:person) OPTIONAL MATCH (b)-[:knows]->(c:person) RETURN COUNT(*)
 ---- 1
 41
+
+-LOG MatchDropsNullFromLeftJoin
+-STATEMENT MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) WITH a, b MATCH (b) RETURN a.ID, b.ID
+---- 3
+0|1
+2|1
+8|1
+
+-LOG MatchDropsNullFromLeftJoinLabel
+-STATEMENT MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) WITH a, b MATCH (b:organisation) RETURN a.ID, b.ID
+---- 3
+0|1
+2|1
+8|1
+
+-LOG MatchDropsNullFromLeftJoinAlias
+-STATEMENT MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) WITH a, b AS c MATCH (c) RETURN a.ID, c.name
+---- 3
+0|ABFsUni
+2|ABFsUni
+8|ABFsUni
+
+-LOG MatchDropsNullFromLeftJoinAfterSet
+-STATEMENT MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) SET a.age = a.age WITH a, b MATCH (b) RETURN a.ID, b.ID
+---- 3
+0|1
+2|1
+8|1
+
+-LOG MatchDropsNullFromOptionalCrossProduct
+-STATEMENT MATCH (a:person) WHERE a.ID < 3 OPTIONAL MATCH (b:organisation) WHERE b.score > 1000 WITH a, b MATCH (b) RETURN a.ID, b.ID
+---- 0
+
+-LOG MatchDropsNullFromOptionalAccumulate
+-STATEMENT OPTIONAL MATCH (b:organisation) WHERE b.score > 1000 WITH b MATCH (b) RETURN b.ID
+---- 0
+
+-LOG MatchDropsNullFromCascadeLast
+-STATEMENT MATCH (a:person) WHERE a.ID IN [0, 8, 10] OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) OPTIONAL MATCH (b)<-[:studyAt]-(c:person) WHERE c.ID > a.ID WITH a, b, c MATCH (c) RETURN a.ID, b.ID, c.ID
+---- 2
+0|1|2
+0|1|8
+
+-LOG MatchDropsNullFromCascadeMiddle
+-STATEMENT MATCH (a:person) WHERE a.ID IN [0, 8, 10] OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) OPTIONAL MATCH (b)<-[:studyAt]-(c:person) WHERE c.ID > a.ID WITH a, b, c MATCH (b) RETURN a.ID, b.ID, c.ID
+---- 3
+0|1|2
+0|1|8
+8|1|
+
+-LOG MatchOnNonNullVariableKeepsRows
+-STATEMENT MATCH (a:person) OPTIONAL MATCH (a)-[:studyAt]->(b:organisation) WITH a, b MATCH (a) RETURN a.ID, b.ID
+---- 8
+0|1
+2|1
+3|
+5|
+7|
+8|1
+9|
+10|