diff --git a/src/validator/GroupByValidator.cpp b/src/validator/GroupByValidator.cpp index de328ac4b..60fc1c11d 100644 --- a/src/validator/GroupByValidator.cpp +++ b/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(agg)) - .ok()) { - return Status::SemanticError("Aggregate function nesting is not allowed: `%s'", - colExpr->toString().c_str()); - } - + NG_RETURN_IF_ERROR( + ExpressionUtils::checkAggExpr(static_cast(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."); diff --git a/src/validator/GroupByValidator.h b/src/validator/GroupByValidator.h index a35be723d..656740ec8 100644 --- a/src/validator/GroupByValidator.h +++ b/src/validator/GroupByValidator.h @@ -30,19 +30,15 @@ class GroupByValidator final : public Validator { Status groupClauseSemanticCheck(); private: - std::vector yieldCols_; - - // key: alias, value: input name - std::unordered_map aliases_; + std::vector groupKeys_; + std::vector groupItems_; + std::vector aggOutputColNames_; bool needGenProject_{false}; - std::vector outputColumnNames_; - // used to generate Project node when there is an internally nested aggregateExpression YieldColumns *projCols_; - - std::vector groupKeys_; - std::vector groupItems_; + // just for groupClauseSemanticCheck + std::vector yieldCols_; }; } // namespace graph diff --git a/tests/tck/features/aggregate/Agg.feature b/tests/tck/features/aggregate/Agg.feature index cf1b65d90..d17a271ae 100644 --- a/tests/tck/features/aggregate/Agg.feature +++ b/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