Cherry pick 3.3 (1022-1025) (#4779)

* Fix/find start error (#4771)

* Fix find start error.

* Fix test.

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* Introduce JSON_EXTRACT function (#4743)

* Introduce JSON_EXTRACT function

close: #3513
Note, we don't support the path argument in this phase

* address jievince's review commit

removed the unecessary interface of construct Map from Value

* Type handling

Only primitive types are supported

* Support depth1 nested

* lint: fmt

* ut: ctest, fixed wrong expression of Map

* fix ut errors

* tck: case for json_extract added

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* Fix bug #1337 from ent (#4740)

* Return an semantic error if no tag is found for a property while validating a match.

* Return an semantic error if no tag is found for a property while validating a match.

* Add a tck case for the fixed bug.

* commented out unused codes.

* add tag in tck cases

* fixing tck

* updated tck cases to add missing cases that are supposed to be there.

* Return an semantic error if no tag is found for a property while validating a match.

* Add a tck case for the fixed bug.

* commented out unused codes.

* add tag in tck cases

* fixing tck

* updated tck cases to add missing cases that are supposed to be there.

* update

* update tck case.

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* Fix RollUpApplyExecutor (#4778)

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* Fix mutil-match crash in optimization phase (#4780)

fmt

small fix

small fix

* fix subgraph step (#4776)

* fix subgraph step

* forbid function call in where clause

* fix error

Co-authored-by: shylock <33566796+Shylock-Hg@users.noreply.github.com>
Co-authored-by: Wey Gu <weyl.gu@gmail.com>
Co-authored-by: Cheng Xuntao <7731943+xtcyclist@users.noreply.github.com>
Co-authored-by: Yichen Wang <18348405+Aiee@users.noreply.github.com>
Co-authored-by: kyle.cao <kyle.cao@vesoft.com>
Co-authored-by: jimingquan <mingquan.ji@vesoft.com>

src/common/datatypes/Map.cpp

@@ -6,6 +6,7 @@
 #include "common/datatypes/Map.h"
 
 #include <folly/String.h>
+#include <folly/json.h>
 
 #include <sstream>
 
@@ -44,6 +45,49 @@ folly::dynamic Map::getMetaData() const {
   return mapMetadataObj;
 }
 
+// Map constructor to covert from folly::dynamic object
+// Called by function: json_extract()
+
+// TODO(wey-gu) support Datetime, deeper nested Map/Datatypes
+Map::Map(const folly::dynamic& obj) {
+  DCHECK(obj.isObject());
+  for (auto& kv : obj.items()) {
+    if (kv.second.isString()) {
+      kvs.emplace(kv.first.asString(), Value(kv.second.asString()));
+    } else if (kv.second.isInt()) {
+      kvs.emplace(kv.first.asString(), Value(kv.second.asInt()));
+    } else if (kv.second.isDouble()) {
+      kvs.emplace(kv.first.asString(), Value(kv.second.asDouble()));
+    } else if (kv.second.isBool()) {
+      kvs.emplace(kv.first.asString(), Value(kv.second.asBool()));
+    } else if (kv.second.isNull()) {
+      kvs.emplace(kv.first.asString(), Value());
+    } else if (kv.second.isObject()) {
+      std::unordered_map<std::string, Value> values;
+      for (auto& nkv : kv.second.items()) {
+        if (nkv.second.isString()) {
+          values.emplace(nkv.first.asString(), Value(nkv.second.asString()));
+        } else if (nkv.second.isInt()) {
+          values.emplace(nkv.first.asString(), Value(nkv.second.asInt()));
+        } else if (nkv.second.isDouble()) {
+          values.emplace(nkv.first.asString(), Value(nkv.second.asDouble()));
+        } else if (nkv.second.isBool()) {
+          values.emplace(nkv.first.asString(), Value(nkv.second.asBool()));
+        } else {
+          LOG(WARNING) << "JSON_EXTRACT nested layer 1: Map can be populated only by "
+                          "Bool, Double, Int, String value and null, now trying to parse from: "
+                       << nkv.second.typeName();
+        }
+      }
+      kvs.emplace(kv.first.asString(), Value(Map(std::move(values))));
+    } else {
+      LOG(WARNING) << "JSON_EXTRACT Only Bool, Double, Int, String value, null and Map(depth==1) "
+                      "are supported, now trying to parse from: "
+                   << kv.second.typeName();
+    }
+  }
+}
+
 }  // namespace nebula
 
 namespace std {

src/common/datatypes/Map.h

@@ -8,6 +8,7 @@
 
 #include <unordered_map>
 
+#include "common/base/Logging.h"
 #include "common/datatypes/Value.h"
 
 namespace nebula {
@@ -22,6 +23,8 @@ struct Map {
     kvs = std::move(values);
   }
 
+  explicit Map(const folly::dynamic& obj);
+
   Map& operator=(const Map& rhs) {
     if (this == &rhs) {
       return *this;

src/common/function/FunctionManager.cpp

@@ -5,6 +5,8 @@
 
 #include "FunctionManager.h"
 
+#include <folly/json.h>
+
 #include <boost/algorithm/string/replace.hpp>
 
 #include "common/base/Base.h"
@@ -421,6 +423,9 @@ std::unordered_map<std::string, std::vector<TypeSignature>> FunctionManager::typ
       TypeSignature({Value::Type::MAP}, Value::Type::DURATION)}},
     {"extract", {TypeSignature({Value::Type::STRING, Value::Type::STRING}, Value::Type::LIST)}},
     {"_nodeid", {TypeSignature({Value::Type::PATH, Value::Type::INT}, Value::Type::INT)}},
+    {"json_extract",
+     {TypeSignature({Value::Type::STRING}, Value::Type::MAP),
+      TypeSignature({Value::Type::STRING}, Value::Type::NULLVALUE)}},
 };
 
 // static
@@ -2766,6 +2771,34 @@ FunctionManager::FunctionManager() {
       }
     };
   }
+  {
+    auto &attr = functions_["json_extract"];
+    // note, we don't support second argument(path) like MySQL JSON_EXTRACT for now
+    attr.minArity_ = 1;
+    attr.maxArity_ = 1;
+    attr.isAlwaysPure_ = true;
+    attr.body_ = [](const auto &args) -> Value {
+      if (!args[0].get().isStr()) {
+        return Value::kNullBadType;
+      }
+      auto json = args[0].get().getStr();
+
+      // invalid string to json will be caught and returned as null
+      try {
+        auto obj = folly::parseJson(json);
+        if (!obj.isObject()) {
+          return Value::kNullBadData;
+        }
+        // if obj is empty, i.e. "{}", return empty map
+        if (obj.empty()) {
+          return Map();
+        }
+        return Map(obj);
+      } catch (const std::exception &e) {
+        return Value::kNullBadData;
+      }
+    };
+  }
 }  // NOLINT
 
 // static

src/common/function/test/FunctionManagerTest.cpp

@@ -132,7 +132,11 @@ std::unordered_map<std::string, std::vector<Value>> FunctionManagerTest::args_ =
     {"date", {Date(1984, 10, 11)}},
     {"datetime", {DateTime(1984, 10, 11, 12, 31, 14, 341)}},
     {"edge", {Edge("1", "2", -1, "e1", 0, {{"e1", 1}, {"e2", 2}})}},
-};
+    {"json_extract0", {"{\"a\": 1, \"b\": 0.2}"}},
+    {"json_extract1", {"{\"a\": 1, \"b\": 0.2, \"c\": {\"d\": true}}"}},
+    {"json_extract2", {"_"}},
+    {"json_extract3", {"{a: 1, \"b\": 0.2}"}},
+    {"json_extract4", {"{\"a\": \"foo\", \"b\": 0.2, \"c\": {\"d\": {\"e\": 0.1}}}"}}};
 
 #define TEST_FUNCTION(expr, ...)                   \
   do {                                             \
@@ -385,6 +389,21 @@ TEST_F(FunctionManagerTest, functionCall) {
   { TEST_FUNCTION(rand32, args_["empty"]); }
   { TEST_FUNCTION(now, args_["empty"]); }
   { TEST_FUNCTION(hash, args_["string"]); }
+  {
+    TEST_FUNCTION(
+        json_extract, args_["json_extract0"], Value(Map({{"a", Value(1)}, {"b", Value(0.2)}})));
+    TEST_FUNCTION(
+        json_extract,
+        args_["json_extract1"],
+        Value(Map({{"a", Value(1)}, {"b", Value(0.2)}, {"c", Value(Map({{"d", Value(true)}}))}})));
+    // invalid json string
+    TEST_FUNCTION(json_extract, args_["json_extract2"], Value::kNullBadData);
+    TEST_FUNCTION(json_extract, args_["json_extract3"], Value::kNullBadData);
+    // when there is nested Map in depth >= 2, the value will be dropped as empty Map()
+    TEST_FUNCTION(json_extract,
+                  args_["json_extract4"],
+                  Value(Map({{"a", Value("foo")}, {"b", Value(0.2)}, {"c", Value(Map())}})));
+  }
   {
     auto result = FunctionManager::get("hash", 1);
     ASSERT_TRUE(result.ok());

src/graph/executor/algo/SubgraphExecutor.cpp

@@ -92,12 +92,7 @@ folly::Future<Status> SubgraphExecutor::handleResponse(RpcResponse&& resps) {
   auto listVal = std::make_shared<Value>(std::move(list));
   auto iter = std::make_unique<GetNeighborsIter>(listVal);
 
-  auto steps = totalSteps_;
-  if (!subgraph_->oneMoreStep()) {
-    --steps;
-  }
-
-  if (!process(std::move(iter)) || ++currentStep_ > steps) {
+  if (!process(std::move(iter)) || ++currentStep_ > totalSteps_) {
     filterEdges(0);
     return folly::makeFuture<Status>(Status::OK());
   } else {

src/graph/executor/query/RollUpApplyExecutor.cpp

@@ -142,17 +142,21 @@ folly::Future<Status> RollUpApplyExecutor::rollUpApply() {
   NG_RETURN_IF_ERROR(checkBiInputDataSets());
   DataSet result;
   mv_ = movable(node()->inputVars()[0]);
+
   if (rollUpApplyNode->compareCols().size() == 0) {
     List hashTable;
     buildZeroKeyHashTable(rollUpApplyNode->collectCol(), rhsIter_.get(), hashTable);
     result = probeZeroKey(lhsIter_.get(), hashTable);
   } else if (rollUpApplyNode->compareCols().size() == 1) {
     std::unordered_map<Value, List> hashTable;
-    buildSingleKeyHashTable(rollUpApplyNode->compareCols()[0],
+    // Clone the expression so when evaluating the InputPropertyExpression, the propIndex_ will not
+    // be buffered.
+    buildSingleKeyHashTable(rollUpApplyNode->compareCols()[0]->clone(),
                             rollUpApplyNode->collectCol(),
                             rhsIter_.get(),
                             hashTable);
-    result = probeSingleKey(rollUpApplyNode->compareCols()[0], lhsIter_.get(), hashTable);
+
+    result = probeSingleKey(rollUpApplyNode->compareCols()[0]->clone(), lhsIter_.get(), hashTable);
   } else {
     std::unordered_map<List, List> hashTable;
     buildHashTable(

src/graph/planner/match/ArgumentFinder.h

@@ -26,6 +26,10 @@ class ArgumentFinder final : public StartVidFinder {
 
   StatusOr<SubPlan> transformEdge(EdgeContext* edgeCtx) override;
 
+  const char* name() const override {
+    return "ArgumentFinder";
+  }
+
  private:
   ArgumentFinder() = default;
 };

src/graph/planner/match/LabelIndexSeek.cpp

@@ -91,7 +91,7 @@ StatusOr<SubPlan> LabelIndexSeek::transformNode(NodeContext* nodeCtx) {
 
     if (filter->kind() == Expression::Kind::kLogicalOr) {
       auto exprs = ExpressionUtils::collectAll(filter, {Expression::Kind::kLabelTagProperty});
-      bool matched = true;
+      bool matched = exprs.empty() ? false : true;
       for (auto* expr : exprs) {
         auto tagPropExpr = static_cast<const LabelTagPropertyExpression*>(expr);
         if (static_cast<const PropertyExpression*>(tagPropExpr->label())->prop() != nodeAlias ||

src/graph/planner/match/LabelIndexSeek.h

@@ -18,6 +18,10 @@ class LabelIndexSeek final : public StartVidFinder {
     return std::unique_ptr<LabelIndexSeek>(new LabelIndexSeek());
   }
 
+  const char* name() const override {
+    return "LabelIndexSeekFinder";
+  }
+
  private:
   LabelIndexSeek() = default;
 

src/graph/planner/match/PropIndexSeek.h

@@ -26,6 +26,10 @@ class PropIndexSeek final : public StartVidFinder {
 
   StatusOr<SubPlan> transformEdge(EdgeContext* edgeCtx) override;
 
+  const char* name() const override {
+    return "PropIndexSeekFinder";
+  }
+
  private:
   PropIndexSeek() = default;
 };

src/graph/planner/match/ScanSeek.h

@@ -26,6 +26,10 @@ class ScanSeek final : public StartVidFinder {
 
   StatusOr<SubPlan> transformEdge(EdgeContext* edgeCtx) override;
 
+  const char* name() const override {
+    return "ScanSeekFinder";
+  }
+
  private:
   ScanSeek() = default;
 };

src/graph/planner/match/StartVidFinder.h

@@ -61,6 +61,8 @@ class StartVidFinder {
 
   virtual StatusOr<SubPlan> transformEdge(EdgeContext* edgeCtx) = 0;
 
+  virtual const char* name() const = 0;
+
  protected:
   StartVidFinder() = default;
 };

src/graph/planner/match/VertexIdSeek.h

@@ -30,6 +30,10 @@ class VertexIdSeek final : public StartVidFinder {
 
   std::string listToAnnoVarVid(QueryContext* qctx, const List& list);
 
+  const char* name() const override {
+    return "VertexIdSeekFinder";
+  }
+
  private:
   VertexIdSeek() = default;
 };

src/graph/planner/ngql/SubgraphPlanner.cpp

@@ -69,7 +69,6 @@ StatusOr<std::unique_ptr<std::vector<EdgeProp>>> SubgraphPlanner::buildEdgeProps
 StatusOr<SubPlan> SubgraphPlanner::nSteps(SubPlan& startVidPlan, const std::string& input) {
   auto* qctx = subgraphCtx_->qctx;
   const auto& space = subgraphCtx_->space;
-  const auto& dstTagProps = subgraphCtx_->exprProps.dstTagProps();
   const auto& steps = subgraphCtx_->steps;
 
   auto vertexProps = buildVertexProps();
@@ -89,9 +88,6 @@ StatusOr<SubPlan> SubgraphPlanner::nSteps(SubPlan& startVidPlan, const std::stri
   subgraph->setEdgeProps(std::move(edgeProps).value());
   subgraph->setInputVar(input);
   subgraph->setBiDirectEdgeTypes(subgraphCtx_->biDirectEdgeTypes);
-  if (subgraphCtx_->getEdgeProp || subgraphCtx_->withProp || !dstTagProps.empty()) {
-    subgraph->setOneMoreStep();
-  }
 
   auto* dc = DataCollect::make(qctx, DataCollect::DCKind::kSubgraph);
   dc->addDep(subgraph);

src/graph/planner/plan/PlanNode.cpp

@@ -423,7 +423,11 @@ BinaryInputNode::BinaryInputNode(QueryContext* qctx,
   }
 
   addDep(right);
-  readVariable(right->outputVarPtr());
+  if (right != nullptr) {
+    readVariable(right->outputVarPtr());
+  } else {
+    inputVars_.emplace_back(nullptr);
+  }
 }
 
 // It's used for clone

src/graph/planner/plan/Query.cpp

@@ -870,7 +870,9 @@ std::unique_ptr<PlanNodeDescription> BiLeftJoin::explain() const {
 }
 
 PlanNode* BiLeftJoin::clone() const {
-  auto* newLeftJoin = BiLeftJoin::make(qctx_, nullptr, nullptr);
+  auto* lnode = left() ? left()->clone() : nullptr;
+  auto* rnode = right() ? right()->clone() : nullptr;
+  auto* newLeftJoin = BiLeftJoin::make(qctx_, lnode, rnode);
   newLeftJoin->cloneMembers(*this);
   return newLeftJoin;
 }
@@ -886,7 +888,9 @@ std::unique_ptr<PlanNodeDescription> BiInnerJoin::explain() const {
 }
 
 PlanNode* BiInnerJoin::clone() const {
-  auto* newInnerJoin = BiInnerJoin::make(qctx_, nullptr, nullptr);
+  auto* lnode = left() ? left()->clone() : nullptr;
+  auto* rnode = right() ? right()->clone() : nullptr;
+  auto* newInnerJoin = BiInnerJoin::make(qctx_, lnode, rnode);
   newInnerJoin->cloneMembers(*this);
   return newInnerJoin;
 }
@@ -925,7 +929,9 @@ void RollUpApply::cloneMembers(const RollUpApply& r) {
 }
 
 PlanNode* RollUpApply::clone() const {
-  auto* newRollUpApply = RollUpApply::make(qctx_, nullptr, nullptr, {}, nullptr);
+  auto* lnode = left() ? left()->clone() : nullptr;
+  auto* rnode = right() ? right()->clone() : nullptr;
+  auto* newRollUpApply = RollUpApply::make(qctx_, lnode, rnode, {}, nullptr);
   newRollUpApply->cloneMembers(*this);
   return newRollUpApply;
 }

src/graph/validator/MatchValidator.cpp

@@ -192,6 +192,12 @@ Status MatchValidator::buildNodeInfo(const MatchPath *path,
     auto alias = node->alias();
     auto *props = node->props();
     auto anonymous = false;
+    // if there exists some property with no tag, return a semantic error
+    if (props != nullptr) {
+      return Status::SemanticError("`%s:%s': No tag found for property.",
+                                   props->items()[0].first.c_str(),
+                                   props->items()[0].second->toString().c_str());
+    }
     if (node->labels() != nullptr) {
       auto &labels = node->labels()->labels();
       for (const auto &label : labels) {
@@ -213,11 +219,18 @@ Status MatchValidator::buildNodeInfo(const MatchPath *path,
       nodeAliases.emplace(alias, AliasType::kNode);
     }
     Expression *filter = nullptr;
+    /* Note(Xuntao): Commented out the following part. With the current parser,
+       if no tag is given in match clauses, node->props() is not nullptr but
+       node-labels() is. This is not supposed to be valid.
+    */
+    /*
     if (props != nullptr) {
       auto result = makeNodeSubFilter(const_cast<MapExpression *>(props), "*");
       NG_RETURN_IF_ERROR(result);
       filter = result.value();
-    } else if (node->labels() != nullptr && !node->labels()->labels().empty()) {
+    } else
+    */
+    if (node->labels() != nullptr && !node->labels()->labels().empty()) {
       const auto &labels = node->labels()->labels();
       for (const auto &label : labels) {
         auto result = makeNodeSubFilter(label->props(), *label->label());

tests/tck/features/bugfix/FindStartError.feature

@@ -0,0 +1,16 @@
+# Copyright (c) 2022 vesoft inc. All rights reserved.
+#
+# This source code is licensed under Apache 2.0 License.
+Feature: Test find start error for property index seek
+
+  Background:
+    Given a graph with space named "nba"
+
+  # #4763
+  Scenario: Find start of match pattern
+    When executing query:
+      """
+      WITH 1 as a MATCH (v:player) WHERE a == 3 OR (a + 1) == 4 RETURN v.player.name;
+      """
+    Then the result should be, in any order:
+      | v.player.name |

tests/tck/features/function/coalesce.feature

@@ -1,4 +1,4 @@
-Feature: Fetch Int Vid Edges
+Feature: Coalesce Function
 
   Background:
     Test coalesce function