From 6bc61a699966c19a3cf9db56b9bf74b78f88a06c Mon Sep 17 00:00:00 2001 From: xtcyclist <7731943+xtcyclist@users.noreply.github.com> Date: Thu, 1 Dec 2022 23:28:48 +0800 Subject: [PATCH] reorged match validator, added a new orderby validator. --- src/graph/validator/MatchValidator.cpp | 147 +++++++++++++---------- src/graph/validator/MatchValidator.h | 6 + tests/tck/features/match/OrderBy.feature | 16 +++ 3 files changed, 107 insertions(+), 62 deletions(-) diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index f07c5fcb4f6..524f576ee03 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -463,64 +463,12 @@ Status MatchValidator::validateReturn(MatchReturn *ret, } } DCHECK(!columns->empty()); - bool preValidatedOrderBy = false; - if (ret->orderFactors() != nullptr && !retClauseCtx.yield->hasAgg_ && !ret->isDistinct() && - !ret->returnItems()->allNamedAliases() && !queryParts.empty()) { + if (ret->orderFactors() != nullptr) { auto orderByCtx = getContext(); - // if the columns required are not already in the projection columns, they may be found - // in in the columns before the projection - YieldColumns *allQueryPartsColumns = nullptr; - std::vector inputColList; - inputColList.reserve(columns->columns().size()); - for (auto *col : columns->columns()) { - inputColList.emplace_back(col->name()); - } - std::unordered_map inputColIndices; - for (auto i = 0u; i < inputColList.size(); i++) { - if (!inputColIndices.emplace(inputColList[i], i).second) { - return Status::SemanticError("Duplicated columns not allowed: %s", inputColList[i].c_str()); - } - } - for (auto &factor : ret->orderFactors()->factors()) { - auto factorExpr = factor->expr(); - if (ExpressionUtils::isEvaluableExpr(factorExpr, qctx_)) continue; - if (factorExpr->kind() != Expression::Kind::kLabel) { - return Status::SemanticError("Only column name can be used as sort item"); - } - auto &name = static_cast(factor->expr())->name(); - auto iter = inputColIndices.find(name); - if (iter == inputColIndices.end()) { - // some column is not found - bool found = false; - if (allQueryPartsColumns == nullptr) { - allQueryPartsColumns = retClauseCtx.qctx->objPool()->makeAndAdd(); - auto status = buildColumnsForAllNamedAliases(queryParts, allQueryPartsColumns); - if (!status.ok()) { - return status; - } - } - for (auto &col : allQueryPartsColumns->columns()) { - if (col->name().compare(name) != 0) { - continue; - } else { - col->setVisible(false); - columns->addColumn(col->clone().release()); - found = true; - orderByCtx->indexedOrderFactors.emplace_back(columns->size() - 1, factor->orderType()); - break; - } - } - if (!found) { - return Status::SemanticError("Column `%s' not found", name.c_str()); - } - } else { - // iter found - orderByCtx->indexedOrderFactors.emplace_back(iter->second, factor->orderType()); - } - } + NG_RETURN_IF_ERROR(validateOrderBy(ret, retClauseCtx, columns, *orderByCtx, queryParts)); retClauseCtx.order = std::move(orderByCtx); - preValidatedOrderBy = true; } + // finished producing columns for further validations retClauseCtx.yield->yieldColumns = columns; @@ -533,13 +481,14 @@ Status MatchValidator::validateReturn(MatchReturn *ret, NG_RETURN_IF_ERROR(validatePagination(ret->skip(), ret->limit(), *paginationCtx)); retClauseCtx.pagination = std::move(paginationCtx); - if (!preValidatedOrderBy && ret->orderFactors() != nullptr) { - auto orderByCtx = getContext(); - NG_RETURN_IF_ERROR( - validateOrderBy(ret->orderFactors(), retClauseCtx.yield->yieldColumns, *orderByCtx)); - retClauseCtx.order = std::move(orderByCtx); - } - + /* + if (!preValidatedOrderBy && ret->orderFactors() != nullptr) { + auto orderByCtx = getContext(); + NG_RETURN_IF_ERROR( + validateOrderBy(ret->orderFactors(), retClauseCtx.yield->yieldColumns, *orderByCtx)); + retClauseCtx.order = std::move(orderByCtx); + } + */ return Status::OK(); } @@ -890,6 +839,80 @@ Status MatchValidator::validateOrderBy(const OrderFactors *factors, return Status::OK(); } +Status MatchValidator::validateOrderBy(MatchReturn *ret, + ReturnClauseContext &retClauseCtx, + YieldColumns *yieldColumns, + OrderByClauseContext &orderByCtx, + const std::vector &queryParts) const { + const OrderFactors *factors = ret->orderFactors(); + bool needsAndOkToExpandScope = !retClauseCtx.yield->hasAgg_ && !ret->isDistinct() && + !ret->returnItems()->allNamedAliases() && !queryParts.empty(); + + // if the columns required are not already in the projection columns, they may be found + // in in the columns before the projection + std::vector inputColList; + inputColList.reserve(yieldColumns->columns().size()); + for (auto *col : yieldColumns->columns()) { + inputColList.emplace_back(col->name()); + } + std::unordered_map inputColIndices; + for (auto i = 0u; i < inputColList.size(); i++) { + if (!inputColIndices.emplace(inputColList[i], i).second) { + return Status::SemanticError("Duplicated columns not allowed: %s", inputColList[i].c_str()); + } + } + YieldColumns *allQueryPartsColumns = nullptr; + for (auto &factor : factors->factors()) { + auto factorExpr = factor->expr(); + if (ExpressionUtils::isEvaluableExpr(factorExpr, qctx_)) continue; + if (factorExpr->kind() != Expression::Kind::kLabel) { + return Status::SemanticError("Only column name can be used as sort item"); + } + auto &name = static_cast(factor->expr())->name(); + auto iter = inputColIndices.find(name); + if (iter == inputColIndices.end()) { + // some column is not found + if (!needsAndOkToExpandScope) { + return Status::SemanticError("Column `%s' not found", name.c_str()); + } else { + // expand the scope to columns before the projection + // and see whether the required column could be found + bool found = false; + if (allQueryPartsColumns == nullptr) { + // expand + allQueryPartsColumns = retClauseCtx.qctx->objPool()->makeAndAdd(); + auto status = buildColumnsForAllNamedAliases(queryParts, allQueryPartsColumns); + if (!status.ok()) { + return status; + } + } + for (auto &col : allQueryPartsColumns->columns()) { + // check column + if (col->name().compare(name) != 0) { + continue; + } else { + col->setVisible(false); + yieldColumns->addColumn(col->clone().release()); + found = true; + orderByCtx.indexedOrderFactors.emplace_back(yieldColumns->size() - 1, + factor->orderType()); + break; + } + } + if (!found) { + return Status::SemanticError("Column `%s' not found", name.c_str()); + } + } + } else { + // iter found + orderByCtx.indexedOrderFactors.emplace_back(iter->second, factor->orderType()); + } + } + // retClauseCtx.order = std::move(orderByCtx); + + return Status::OK(); +} + // Validate group by and fill group by context. Status MatchValidator::validateGroup(YieldClauseContext &yieldCtx) { auto cols = yieldCtx.yieldColumns->columns(); diff --git a/src/graph/validator/MatchValidator.h b/src/graph/validator/MatchValidator.h index b33feede7dc..ffb537f4a22 100644 --- a/src/graph/validator/MatchValidator.h +++ b/src/graph/validator/MatchValidator.h @@ -54,6 +54,12 @@ class MatchValidator final : public Validator { const YieldColumns *yieldColumns, OrderByClauseContext &orderByCtx) const; + Status validateOrderBy(MatchReturn *ret, + ReturnClauseContext &retClauseCtx, + YieldColumns *yieldColumns, + OrderByClauseContext &orderByCtx, + const std::vector &queryParts) const; + Status validateGroup(YieldClauseContext &yieldCtx); Status validateYield(YieldClauseContext &yieldCtx); diff --git a/tests/tck/features/match/OrderBy.feature b/tests/tck/features/match/OrderBy.feature index 266966ac7d5..6ea757b136d 100644 --- a/tests/tck/features/match/OrderBy.feature +++ b/tests/tck/features/match/OrderBy.feature @@ -121,3 +121,19 @@ Feature: OrderBy in Match | "Tim Duncan" | 2 | | "Tim Duncan" | 2 | | "Manu Ginobili" | 2 | + When executing query: + """ + match (v:player)-->(v1:player) + with v, v1, count(v) as a0 + where a0 > 1 + return * + order by a0 desc + """ + Then the result should be, in order: + | v | v1 | a0 | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | ("LaMarcus Aldridge" :player{age: 33, name: "LaMarcus Aldridge"}) | 2 | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | 2 | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) | 2 | + | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | 2 | + | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) | 2 | + | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | 2 |