fix agg type check when nested (#1186)

* fix agg type check when nested

* add tck

src/validator/GroupByValidator.cpp

@@ -43,15 +43,11 @@ Status GroupByValidator::validateYield(const YieldClause* yieldClause) {
                 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());
-                }
-
+                NG_RETURN_IF_ERROR(
+                    ExpressionUtils::checkAggExpr(static_cast<const AggregateExpression*>(agg)));
+                aggOutputColNames_.emplace_back(agg->toString());
                 groupItems_.emplace_back(agg->clone());
                 needGenProject_ = true;
-                outputColumnNames_.emplace_back(agg->toString());
             }
             if (!aggs.empty()) {
                 auto* colRewrited = ExpressionUtils::rewriteAgg2VarProp(pool, colExpr);
@@ -69,23 +65,14 @@ Status GroupByValidator::validateYield(const YieldClause* yieldClause) {
         }
 
         groupItems_.emplace_back(colExpr);
-
-        auto status = deduceExprType(colExpr);
-        NG_RETURN_IF_ERROR(status);
-        auto type = std::move(status).value();
+        aggOutputColNames_.emplace_back(colOldName);
 
         projCols_->addColumn(
             new YieldColumn(VariablePropertyExpression::make(pool, "", colOldName), colOldName));
 
-        outputs_.emplace_back(colOldName, type);
-        outputColumnNames_.emplace_back(colOldName);
-
-        if (!col->alias().empty()) {
-            aliases_.emplace(col->alias(), col);
-        }
-
         ExpressionProps yieldProps;
         NG_RETURN_IF_ERROR(deduceProps(colExpr, yieldProps));
+        // TODO: refactor exprProps_ related logic
         exprProps_.unionProps(std::move(yieldProps));
     }
 
@@ -132,7 +119,7 @@ Status GroupByValidator::validateGroup(const GroupClause* groupClause) {
 
 Status GroupByValidator::toPlan() {
     auto* groupBy = Aggregate::make(qctx_, nullptr, std::move(groupKeys_), std::move(groupItems_));
-    groupBy->setColNames(outputColumnNames_);
+    groupBy->setColNames(aggOutputColNames_);
     if (needGenProject_) {
         // rewrite Expr which has inner aggExpr and push it up to Project.
         root_ = Project::make(qctx_, groupBy, projCols_);
@@ -144,6 +131,13 @@ Status GroupByValidator::toPlan() {
 }
 
 Status GroupByValidator::groupClauseSemanticCheck() {
+    // deduce group items and build outputs_
+    DCHECK_EQ(aggOutputColNames_.size(), groupItems_.size());
+    for (auto i = 0u; i < groupItems_.size(); ++i) {
+        auto type = deduceExprType(groupItems_[i]);
+        NG_RETURN_IF_ERROR(type);
+        outputs_.emplace_back(aggOutputColNames_[i], std::move(type).value());
+    }
     // check exprProps
     if (!exprProps_.srcTagProps().empty() || !exprProps_.dstTagProps().empty()) {
         return Status::SemanticError("Only support input and variable in GroupBy sentence.");

src/validator/GroupByValidator.h

@@ -30,19 +30,15 @@ class GroupByValidator final : public Validator {
     Status groupClauseSemanticCheck();
 
 private:
-    std::vector<Expression *> yieldCols_;
-
-    // key: alias, value: input name
-    std::unordered_map<std::string, YieldColumn *> aliases_;
+    std::vector<Expression *> groupKeys_;
+    std::vector<Expression *> groupItems_;
 
+    std::vector<std::string> aggOutputColNames_;
     bool needGenProject_{false};
-    std::vector<std::string> outputColumnNames_;
-
     // used to generate Project node when there is an internally nested aggregateExpression
     YieldColumns *projCols_;
-
-    std::vector<Expression *> groupKeys_;
-    std::vector<Expression *> groupItems_;
+    // just for groupClauseSemanticCheck
+    std::vector<Expression *> yieldCols_;
 };
 
 }   // namespace graph

tests/tck/features/aggregate/Agg.feature

@@ -33,6 +33,13 @@ Feature: Basic Aggregate and GroupBy
     Then the result should be, in any order, with relax comparison:
       | v1 | v2   | v3 | v4   | v5   | v6   | v9   | v10  | v11  |
       | 0  | NULL | 0  | NULL | NULL | NULL | NULL | NULL | NULL |
+    When executing query:
+      """
+      YIELD 3 AS x | YIELD case COUNT($-.x) == 1 when true THEN 1 ELSE 0 END as a
+      """
+    Then the result should be, in any order, with relax comparison:
+      | a |
+      | 1 |
 
   Scenario: [1] Basic GroupBy
     When executing query:
@@ -600,6 +607,11 @@ Feature: Basic Aggregate and GroupBy
       | GROUP BY $-.dst,$-.x YIELD avg(distinct $-.age) AS age
       """
     Then a SemanticError should be raised at runtime:  `$-.x', not exist prop `x'
+    When executing query:
+      """
+      YIELD case COUNT($-.x) == 1 when true THEN 1 ELSE 0 END as a
+      """
+    Then a SemanticError should be raised at runtime:  `$-.x', not exist prop `x'
     When executing query:
       """
       GO FROM "Tim Duncan" OVER like YIELD like._dst AS dst, $$.player.age AS age
@@ -623,7 +635,7 @@ Feature: Basic Aggregate and GroupBy
       GO FROM "Tim Duncan" OVER like YIELD like._dst AS dst, $$.player.age AS age
       | GROUP BY $-.age+1 YIELD $-.age+1,abs(avg(distinct count($-.age))) AS age
       """
-    Then a SemanticError should be raised at runtime: Aggregate function nesting is not allowed: `abs(avg(distinct count($-.age)))'
+    Then a SemanticError should be raised at runtime: Aggregate function nesting is not allowed: `avg(distinct count($-.age))'
     When executing query:
       """
       GO FROM "Tim Duncan" OVER like YIELD like._dst AS dst, $$.player.age AS age