fix bug：Using the same statement to return same vertex different prop…

…erties, the results show BAD TYPE. (#4151) * fix bug * temp commit * temp commit * temp commit * temp commit * temp commit * temp commit * temp commit * complete code * test * temp commit * complete * add ut * add tck * fix comment * fix tck * complete * complete * temp commit * temp commit * temp commit * temp commit
vesoft-inc · Apr 26, 2022 · 4026d75 · 4026d75
1 parent 5b9a7f8
commit 4026d75
Show file tree

Hide file tree

Showing 12 changed files with 809 additions and 15 deletions.
diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h
@@ -115,6 +115,12 @@ struct PaginationContext final : CypherClauseContextBase {
   int64_t limit{std::numeric_limits<int64_t>::max()};
 };
 
+// Used to handle implicit groupBy
+struct GroupSuite {
+  std::vector<Expression*> groupKeys;
+  std::vector<Expression*> groupItems;
+};
+
 struct YieldClauseContext final : CypherClauseContextBase {
   YieldClauseContext() : CypherClauseContextBase(CypherClauseKind::kYield) {}
 
@@ -228,6 +234,7 @@ struct EdgeContext final : PatternContext {
   // initialize start expression in project node
   Expression* initialExpr{nullptr};
 };
+
 }  // namespace graph
 }  // namespace nebula
 #endif  // GRAPH_CONTEXT_AST_CYPHERASTCONTEXT_H_
diff --git a/src/graph/util/ExpressionUtils.cpp b/src/graph/util/ExpressionUtils.cpp
@@ -189,6 +189,24 @@ Expression *ExpressionUtils::rewriteAgg2VarProp(const Expression *expr) {
   return RewriteVisitor::transform(expr, std::move(matcher), std::move(rewriter));
 }
 
+Expression *ExpressionUtils::rewriteSubExprs2VarProp(const Expression *expr,
+                                                     std::vector<Expression *> &subExprs) {
+  ObjectPool *pool = expr->getObjPool();
+  auto matcher = [&subExprs](const Expression *e) -> bool {
+    for (auto subExpr : subExprs) {
+      if (e->toString() == subExpr->toString()) {
+        return true;
+      }
+    }
+    return false;
+  };
+  auto rewriter = [pool](const Expression *e) -> Expression * {
+    return VariablePropertyExpression::make(pool, "", e->toString());
+  };
+
+  return RewriteVisitor::transform(expr, std::move(matcher), std::move(rewriter));
+}
+
 // Rewrite the IN expr to a relEQ expr if the right operand has only 1 element.
 // Rewrite the IN expr to an OR expr if the right operand has more than 1 element.
 Expression *ExpressionUtils::rewriteInExpr(const Expression *expr) {

diff --git a/src/graph/util/ExpressionUtils.h b/src/graph/util/ExpressionUtils.h
@@ -73,6 +73,10 @@ class ExpressionUtils {
   // rewrite Agg to VarProp
   static Expression* rewriteAgg2VarProp(const Expression* expr);
 
+  // rewrite subExprs to VariablePropertyExpression
+  static Expression* rewriteSubExprs2VarProp(const Expression* expr,
+                                             std::vector<Expression*>& subExprs);
+
   // rewrite var in VariablePropExpr to another var
   static Expression* rewriteInnerVar(const Expression* expr, std::string newVar);
 

diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp
@@ -7,6 +7,7 @@
 
 #include "graph/planner/match/MatchSolver.h"
 #include "graph/util/ExpressionUtils.h"
+#include "graph/visitor/ExtractGroupSuiteVisitor.h"
 #include "graph/visitor/RewriteVisitor.h"
 
 namespace nebula {
@@ -781,23 +782,27 @@ Status MatchValidator::validateGroup(YieldClauseContext &yieldCtx) {
     auto *colExpr = col->expr();
     NG_RETURN_IF_ERROR(validateMatchPathExpr(colExpr, yieldCtx.aliasesAvailable, yieldCtx.paths));
     auto colOldName = col->name();
-    if (colExpr->kind() != Expression::Kind::kAggregate) {
-      auto collectAggCol = colExpr->clone();
-      auto aggs = ExpressionUtils::collectAll(collectAggCol, {Expression::Kind::kAggregate});
-      for (auto *agg : aggs) {
-        DCHECK_EQ(agg->kind(), Expression::Kind::kAggregate);
-        if (!ExpressionUtils::checkAggExpr(static_cast<const AggregateExpression *>(agg)).ok()) {
-          return Status::SemanticError("Aggregate function nesting is not allowed: `%s'",
-                                       colExpr->toString().c_str());
+    if (colExpr->kind() != Expression::Kind::kAggregate &&
+        ExpressionUtils::hasAny(colExpr, {Expression::Kind::kAggregate})) {
+      ExtractGroupSuiteVisitor visitor(qctx_);
+      colExpr->accept(&visitor);
+      GroupSuite groupSuite = visitor.groupSuite();
+      yieldCtx.groupKeys_.insert(
+          yieldCtx.groupKeys_.end(), groupSuite.groupKeys.begin(), groupSuite.groupKeys.end());
+      for (auto *item : groupSuite.groupItems) {
+        if (item->kind() == Expression::Kind::kAggregate) {
+          NG_RETURN_IF_ERROR(
+              ExpressionUtils::checkAggExpr(static_cast<const AggregateExpression *>(item)));
         }
 
-        yieldCtx.groupItems_.emplace_back(agg->clone());
+        yieldCtx.groupItems_.emplace_back(item);
 
         yieldCtx.needGenProject_ = true;
-        yieldCtx.aggOutputColumnNames_.emplace_back(agg->toString());
+        yieldCtx.aggOutputColumnNames_.emplace_back(item->toString());
       }
-      if (!aggs.empty()) {
-        auto *rewritedExpr = ExpressionUtils::rewriteAgg2VarProp(colExpr->clone());
+      if (!groupSuite.groupItems.empty()) {
+        auto *rewritedExpr =
+            ExpressionUtils::rewriteSubExprs2VarProp(colExpr->clone(), groupSuite.groupItems);
         yieldCtx.projCols_->addColumn(new YieldColumn(rewritedExpr, colOldName));
         yieldCtx.projOutputColumnNames_.emplace_back(colOldName);
         continue;

diff --git a/src/graph/visitor/CMakeLists.txt b/src/graph/visitor/CMakeLists.txt
@@ -16,6 +16,7 @@ nebula_add_library(
     FindVisitor.cpp
     VidExtractVisitor.cpp
     EvaluableExprVisitor.cpp
+    ExtractGroupSuiteVisitor.cpp
 )
 
 nebula_add_library(

diff --git a/src/graph/visitor/ExtractGroupSuiteVisitor.cpp b/src/graph/visitor/ExtractGroupSuiteVisitor.cpp
@@ -0,0 +1,200 @@
+/* Copyright (c) 2022 vesoft inc. All rights reserved.
+ *
+ * This source code is licensed under Apache 2.0 License.
+ */
+
+#include "graph/visitor/ExtractGroupSuiteVisitor.h"
+
+#include "graph/util/ExpressionUtils.h"
+
+namespace nebula {
+namespace graph {
+
+void ExtractGroupSuiteVisitor::visit(ConstantExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(UnaryExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(TypeCastingExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(LabelExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(LabelAttributeExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(ArithmeticExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(RelationalExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(SubscriptExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(AttributeExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(LogicalExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(FunctionCallExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(AggregateExpression *expr) {
+  groupSuite_.groupItems.emplace_back(expr->clone());
+}
+
+void ExtractGroupSuiteVisitor::visit(UUIDExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(VariableExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(VersionedVariableExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(ListExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(SetExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(MapExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(LabelTagPropertyExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(TagPropertyExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(EdgePropertyExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(InputPropertyExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(VariablePropertyExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(DestPropertyExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(SourcePropertyExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(EdgeSrcIdExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(EdgeTypeExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(EdgeRankExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(EdgeDstIdExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(VertexExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(EdgeExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(CaseExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(PathBuildExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(ColumnExpression *expr) {
+  pushGroupSuite(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(PredicateExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(ListComprehensionExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(ReduceExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(SubscriptRangeExpression *expr) {
+  internalVisit(expr);
+}
+
+void ExtractGroupSuiteVisitor::visit(MatchPathPatternExpression *expr) {
+  internalVisit(expr);
+}
+
+template <typename T>
+void ExtractGroupSuiteVisitor::internalVisit(T *expr) {
+  if (ExpressionUtils::hasAny(expr, {Expression::Kind::kAggregate})) {
+    ExprVisitorImpl::visit(expr);
+  } else {
+    if (!ExpressionUtils::isEvaluableExpr(expr, qctx_)) {
+      pushGroupSuite(expr);
+    }
+  }
+}
+
+template <typename T>
+void ExtractGroupSuiteVisitor::pushGroupSuite(T *expr) {
+  // When expr is PredicateExpression, ListComprehensionExpression or ReduceExpression,
+  // it needs to check whether contains innerVar. If so, it doesn't need to push groupKeys and
+  // groupItems.
+  auto specialExprs = ExpressionUtils::collectAll(expr, {Expression::Kind::kVar});
+  for (auto *s : specialExprs) {
+    if (s->kind() == Expression::Kind::kVar &&
+        static_cast<const VariableExpression *>(s)->isInner()) {
+      return;
+    }
+  }
+  if (ExpressionUtils::isEvaluableExpr(expr, qctx_)) {
+    return;
+  }
+  groupSuite_.groupKeys.emplace_back(expr->clone());
+  groupSuite_.groupItems.emplace_back(expr->clone());
+}
+
+}  // namespace graph
+}  // namespace nebula