Fix multiple match joined on edge produce wrong result (#4923)

* Fix multiple match joined on edge produce wrong result

* refine

* refine

* address comments

* fix lint

* fix build

* fix unwind type inference

* add some code comment

* rename to _joinkey

* minor refine

src/common/datatypes/Edge.cpp

@@ -3,6 +3,7 @@
  * This source code is licensed under Apache 2.0 License.
  */
 
+#include <common/base/Logging.h>
 #include <common/datatypes/Edge.h>
 #include <folly/String.h>
 #include <folly/hash/Hash.h>
@@ -141,6 +142,31 @@ bool Edge::keyEqual(const Edge& rhs) const {
   return src == rhs.dst && dst == rhs.src && ranking == rhs.ranking;
 }
 
+std::string Edge::id() const {
+  std::string s;
+  if (src.type() == Value::Type::INT) {
+    EdgeType t = type > 0 ? type : -type;
+    const int64_t& srcId = type > 0 ? src.getInt() : dst.getInt();
+    const int64_t& dstId = type > 0 ? dst.getInt() : src.getInt();
+    s.reserve(sizeof(srcId) + sizeof(dstId) + sizeof(type) + sizeof(ranking));
+    s.append(reinterpret_cast<const char*>(&srcId), sizeof(srcId));
+    s.append(reinterpret_cast<const char*>(&dstId), sizeof(dstId));
+    s.append(reinterpret_cast<const char*>(&t), sizeof(t));
+    s.append(reinterpret_cast<const char*>(&ranking), sizeof(ranking));
+  } else {
+    DCHECK(src.type() == Value::Type::STRING);
+    EdgeType t = type > 0 ? type : -type;
+    const std::string& srcId = type > 0 ? src.getStr() : dst.getStr();
+    const std::string& dstId = type > 0 ? dst.getStr() : src.getStr();
+    s.reserve(srcId.size() + dstId.size() + sizeof(t) + sizeof(ranking));
+    s.append(srcId.data(), srcId.size());
+    s.append(dstId.data(), dstId.size());
+    s.append(reinterpret_cast<const char*>(&t), sizeof(t));
+    s.append(reinterpret_cast<const char*>(&ranking), sizeof(ranking));
+  }
+  return s;
+}
+
 }  // namespace nebula
 
 namespace std {

src/common/datatypes/Edge.h

@@ -72,6 +72,9 @@ struct Edge {
   const Value& value(const std::string& key) const;
 
   bool keyEqual(const Edge& rhs) const;
+
+  // Return this edge's id encoded in string
+  std::string id() const;
 };
 
 inline std::ostream& operator<<(std::ostream& os, const Edge& v) {

src/common/datatypes/test/EdgeTest.cpp

@@ -104,4 +104,43 @@ TEST(Edge, hashEdge) {
   EXPECT_NE(edge1, edge5);
 }
 
+TEST(Edge, id) {
+  {
+    Edge edge1(0, 1, 1, "like", 100, {});
+
+    Edge edge2(0, 1, 1, "like", 100, {});
+    EXPECT_EQ(edge1.id(), edge2.id());
+
+    Edge edge3(1, 1, 1, "like", 100, {});
+    EXPECT_NE(edge1.id(), edge3.id());
+
+    Edge edge4(0, 2, 1, "like", 100, {});
+    EXPECT_NE(edge1.id(), edge4.id());
+
+    Edge edge5(0, 1, -1, "like", 100, {});
+    EXPECT_NE(edge1.id(), edge5.id());
+
+    Edge edge6(0, 1, 1, "like", 101, {});
+    EXPECT_NE(edge1.id(), edge6.id());
+  }
+  {
+    Edge edge1("aaa", "bbb", 1, "like", 100, {});
+
+    Edge edge2("aaa", "bbb", 1, "like", 100, {});
+    EXPECT_EQ(edge1.id(), edge2.id());
+
+    Edge edge3("aab", "bbb", 1, "like", 100, {});
+    EXPECT_NE(edge1.id(), edge3.id());
+
+    Edge edge4("aaa", "bba", 1, "like", 100, {});
+    EXPECT_NE(edge1.id(), edge4.id());
+
+    Edge edge5("aaa", "bbb", 2, "like", 100, {});
+    EXPECT_NE(edge1.id(), edge5.id());
+
+    Edge edge6("aaa", "bbb", 1, "like", 99, {});
+    EXPECT_NE(edge1.id(), edge6.id());
+  }
+}
+
 }  // namespace nebula

src/common/function/FunctionManager.cpp

@@ -1841,6 +1841,33 @@ FunctionManager::FunctionManager() {
       }
     };
   }
+  {
+    auto &attr = functions_["_joinkey"];
+    attr.minArity_ = 1;
+    attr.maxArity_ = 1;
+    attr.isAlwaysPure_ = true;
+    attr.body_ = [](const auto &args) -> Value {
+      const Value &value = args[0].get();
+      switch (value.type()) {
+        case Value::Type::NULLVALUE: {
+          return Value::kNullValue;
+        }
+        case Value::Type::VERTEX: {
+          return value.getVertex().vid;
+        }
+        // NOTE:
+        // id() on Edge is designed to be used get a Join key when
+        // Join operator performed on edge, the returned id is a
+        // string encoded the {src, dst, type, ranking} tuple
+        case Value::Type::EDGE: {
+          return value.getEdge().id();
+        }
+        default: {
+          return Value::kNullBadType;
+        }
+      }
+    };
+  }
   {
     auto &attr = functions_["tags"];
     attr.minArity_ = 1;

src/graph/context/ast/CypherAstContext.h

@@ -55,7 +55,7 @@ struct EdgeInfo {
   Expression* filter{nullptr};
 };
 
-enum class AliasType : int8_t { kNode, kEdge, kPath, kDefault };
+enum class AliasType : int8_t { kNode, kEdge, kPath, kEdgeList, kDefault };
 
 struct ScanInfo {
   Expression* filter{nullptr};

src/graph/planner/match/MatchPathPlanner.cpp

@@ -249,7 +249,7 @@ Status MatchPathPlanner::leftExpandFromNode(
     auto* pool = qctx->objPool();
     auto args = ArgumentList::make(pool);
     args->addArgument(InputPropertyExpression::make(pool, nodeInfos[startIndex].alias));
-    nextTraverseStart = FunctionCallExpression::make(pool, "id", args);
+    nextTraverseStart = FunctionCallExpression::make(pool, "_joinkey", args);
   }
   bool reversely = true;
   for (size_t i = startIndex; i > 0; --i) {

src/graph/planner/match/MatchPlanner.cpp

@@ -70,7 +70,20 @@ Status MatchPlanner::connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* ma
   }
   std::unordered_set<std::string> intersectedAliases;
   for (auto& alias : matchCtx->aliasesGenerated) {
-    if (matchCtx->aliasesAvailable.find(alias.first) != matchCtx->aliasesAvailable.end()) {
+    auto it = matchCtx->aliasesAvailable.find(alias.first);
+    if (it != matchCtx->aliasesAvailable.end()) {
+      // Joined type should be same
+      if (it->second != alias.second) {
+        return Status::SemanticError(fmt::format("{} binding to different type: {} vs {}",
+                                                 alias.first,
+                                                 AliasTypeName[static_cast<int>(alias.second)],
+                                                 AliasTypeName[static_cast<int>(it->second)]));
+      }
+      // Joined On EdgeList is not supported
+      if (alias.second == AliasType::kEdgeList) {
+        return Status::SemanticError(alias.first +
+                                     " defined with type EdgeList, which cannot be joined on");
+      }
       intersectedAliases.insert(alias.first);
     }
   }

src/graph/planner/match/MatchPlanner.h

@@ -23,6 +23,7 @@ class MatchPlanner final : public Planner {
   StatusOr<SubPlan> transform(AstContext* astCtx) override;
 
  private:
+  static constexpr std::array AliasTypeName = {"Node", "Edge", "Path", "EdgeList", "Default"};
   bool tailConnected_{false};
   StatusOr<SubPlan> genPlan(CypherClauseContextBase* clauseCtx);
   Status connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* matchCtx);

src/graph/planner/match/SegmentsConnector.cpp

@@ -23,7 +23,7 @@ SubPlan SegmentsConnector::innerJoin(QueryContext* qctx,
   for (auto& alias : intersectedAliases) {
     auto* args = ArgumentList::make(pool);
     args->addArgument(InputPropertyExpression::make(pool, alias));
-    auto* expr = FunctionCallExpression::make(pool, "id", args);
+    auto* expr = FunctionCallExpression::make(pool, "_joinkey", args);
     hashKeys.emplace_back(expr);
     probeKeys.emplace_back(expr->clone());
   }
@@ -46,7 +46,7 @@ SubPlan SegmentsConnector::leftJoin(QueryContext* qctx,
   for (auto& alias : intersectedAliases) {
     auto* args = ArgumentList::make(pool);
     args->addArgument(InputPropertyExpression::make(pool, alias));
-    auto* expr = FunctionCallExpression::make(pool, "id", args);
+    auto* expr = FunctionCallExpression::make(pool, "_joinkey", args);
     hashKeys.emplace_back(expr);
     probeKeys.emplace_back(expr->clone());
   }

src/graph/validator/MatchValidator.cpp

@@ -283,16 +283,21 @@ Status MatchValidator::buildEdgeInfo(const MatchPath *path,
         edgeInfos[i].types.emplace_back(typeName.value());
       }
     }
+    AliasType aliasType = AliasType::kEdge;
     auto *stepRange = edge->range();
     if (stepRange != nullptr) {
       NG_RETURN_IF_ERROR(validateStepRange(stepRange));
       edgeInfos[i].range = stepRange;
+      // Type of [e*1..2], [e*2] should be inference to EdgeList
+      if (stepRange->max() > stepRange->min() || stepRange->min() > 1) {
+        aliasType = AliasType::kEdgeList;
+      }
     }
     if (alias.empty()) {
       anonymous = true;
       alias = vctx_->anonVarGen()->getVar();
     } else {
-      if (!aliases.emplace(alias, AliasType::kEdge).second) {
+      if (!aliases.emplace(alias, aliasType).second) {
         return Status::SemanticError("`%s': Redefined alias", alias.c_str());
       }
     }
@@ -601,14 +606,28 @@ Status MatchValidator::validateUnwind(const UnwindClause *unwindClause,
   }
 
   auto labelExprs = ExpressionUtils::collectAll(unwindCtx.unwindExpr, {Expression::Kind::kLabel});
+  std::vector<AliasType> types;
   for (auto *labelExpr : labelExprs) {
     DCHECK_EQ(labelExpr->kind(), Expression::Kind::kLabel);
     auto label = static_cast<const LabelExpression *>(labelExpr)->name();
-    if (!unwindCtx.aliasesAvailable.count(label)) {
+    auto it = unwindCtx.aliasesAvailable.find(label);
+    if (it == unwindCtx.aliasesAvailable.end()) {
       return Status::SemanticError("Variable `%s` not defined", label.c_str());
     }
+    types.push_back(it->second);
+  }
+  // UNWIND Type Inference:
+  //   Example: UNWIND x,y AS z
+  //   if x,y have same type
+  //      set z to the same type
+  //   else
+  //      set z to default
+  AliasType aliasType = AliasType::kDefault;
+  if (types.size() > 0 &&
+      std::adjacent_find(types.begin(), types.end(), std::not_equal_to<>()) == types.end()) {
+    aliasType = types[0];
   }
-  unwindCtx.aliasesGenerated.emplace(unwindCtx.alias, AliasType::kDefault);
+  unwindCtx.aliasesGenerated.emplace(unwindCtx.alias, aliasType);
   if (unwindCtx.aliasesAvailable.count(unwindCtx.alias) > 0) {
     return Status::SemanticError("Variable `%s` already declared", unwindCtx.alias.c_str());
   }

tests/tck/features/bugfix/MatchJoinOnEdge.feature

@@ -0,0 +1,32 @@
+# Copyright (c) 2020 vesoft inc. All rights reserved.
+#
+# This source code is licensed under Apache 2.0 License.
+Feature: Test match used in pipe
+
+  Background:
+    Given a graph with space named "nba"
+
+  Scenario: Multiple Match joined on edge
+    When executing query:
+      """
+      MATCH (v:player)-[e:like*1..2]->(u) WHERE v.player.name=="Tim Duncan" MATCH (vv:player)-[e:like]->() WHERE vv.player.name=="Tony Parker"return v, u
+      """
+    Then a SemanticError should be raised at runtime: e binding to different type: Edge vs EdgeList
+    When executing query:
+      """
+      MATCH (v:player)-[e:like*2]->(u) WHERE v.player.name=="Tim Duncan" MATCH (vv:player)-[e:like]->() WHERE vv.player.name=="Tony Parker"return v, u
+      """
+    Then a SemanticError should be raised at runtime: e binding to different type: Edge vs EdgeList
+    When executing query:
+      """
+      MATCH (v:player)-[e:like]->() WHERE v.player.name=="Tim Duncan" MATCH (u:player)-[e:like]->() WHERE u.player.name=="Tony Parker"return v
+      """
+    Then the result should be, in any order:
+      | v |
+    When executing query:
+      """
+      MATCH (v:player)-[e:like]->() WHERE v.player.name=="Tim Duncan" MATCH ()-[e:like]->(u:player) WHERE u.player.name=="Tony Parker"return v, u
+      """
+    Then the result should be, in any order:
+      | v                                                                                                           | u                                                     |
+      | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) |