From aa5fbb475fad2df9c13906f8207310e15028b42c Mon Sep 17 00:00:00 2001 From: "jie.wang" <38901892+jievince@users.noreply.github.com> Date: Wed, 11 Aug 2021 14:51:24 +0800 Subject: [PATCH] [cherry-pick #1222]fix the wrong usage of limit sentence (#1314) * fix the wrong usage of limit sentence * add some tck tests * support order by with variable * address comments --- src/parser/parser.yy | 2 +- src/validator/OrderByValidator.cpp | 55 ++++++++++++++----- src/validator/OrderByValidator.h | 1 + src/validator/SequentialValidator.cpp | 10 ---- .../fetch/FetchVertices.intVid.feature | 2 +- .../fetch/FetchVertices.strVid.feature | 2 +- tests/tck/features/go/GroupbyLimit.feature | 5 ++ tests/tck/features/go/Orderby.feature | 44 ++++++++++++++- tests/tck/features/lookup/ByIndex.feature | 4 +- .../features/lookup/ByIndex.intVid.feature | 4 +- tests/tck/features/optimizer/TopNRule.feature | 8 +-- 11 files changed, 99 insertions(+), 38 deletions(-) diff --git a/src/parser/parser.yy b/src/parser/parser.yy index 8eba1beab..05758d926 100644 --- a/src/parser/parser.yy +++ b/src/parser/parser.yy @@ -2537,7 +2537,6 @@ traverse_sentence | order_by_sentence { $$ = $1; } | fetch_sentence { $$ = $1; } | find_path_sentence { $$ = $1; } - | limit_sentence { $$ = $1; } | yield_sentence { $$ = $1; } | get_subgraph_sentence { $$ = $1; } | delete_vertex_sentence { $$ = $1; } @@ -2549,6 +2548,7 @@ traverse_sentence piped_sentence : traverse_sentence { $$ = $1; } | piped_sentence PIPE traverse_sentence { $$ = new PipedSentence($1, $3); } + | piped_sentence PIPE limit_sentence { $$ = new PipedSentence($1, $3); } ; set_sentence diff --git a/src/validator/OrderByValidator.cpp b/src/validator/OrderByValidator.cpp index ebf52cfab..ddd4c04d0 100644 --- a/src/validator/OrderByValidator.cpp +++ b/src/validator/OrderByValidator.cpp @@ -13,26 +13,47 @@ namespace nebula { namespace graph { Status OrderByValidator::validateImpl() { auto sentence = static_cast(sentence_); - outputs_ = inputCols(); auto &factors = sentence->factors(); - auto *pool = qctx_->objPool(); for (auto &factor : factors) { - if (factor->expr()->kind() == Expression::Kind::kLabel) { - auto *label = static_cast(factor->expr()); - auto *expr = InputPropertyExpression::make(pool, label->name()); - factor->setExpr(expr); - } - if (factor->expr()->kind() != Expression::Kind::kInputProperty) { + if (factor->expr()->kind() == Expression::Kind::kInputProperty) { + auto expr = static_cast(factor->expr()); + NG_RETURN_IF_ERROR(deduceExprType(expr)); + NG_RETURN_IF_ERROR(deduceProps(expr, exprProps_)); + const auto &cols = inputCols(); + auto& name = expr->prop(); + auto eq = [&](const ColDef& col) { return col.name == name; }; + auto iter = std::find_if(cols.cbegin(), cols.cend(), eq); + size_t colIdx = std::distance(cols.cbegin(), iter); + colOrderTypes_.emplace_back(std::make_pair(colIdx, factor->orderType())); + } else if (factor->expr()->kind() == Expression::Kind::kVarProperty) { + auto expr = static_cast(factor->expr()); + NG_RETURN_IF_ERROR(deduceExprType(expr)); + NG_RETURN_IF_ERROR(deduceProps(expr, exprProps_)); + const auto &cols = vctx_->getVar(expr->sym()); + auto& name = expr->prop(); + auto eq = [&](const ColDef& col) { return col.name == name; }; + auto iter = std::find_if(cols.cbegin(), cols.cend(), eq); + size_t colIdx = std::distance(cols.cbegin(), iter); + colOrderTypes_.emplace_back(std::make_pair(colIdx, factor->orderType())); + } else { return Status::SemanticError("Order by with invalid expression `%s'", factor->expr()->toString().c_str()); } - auto expr = static_cast(factor->expr()); - NG_RETURN_IF_ERROR(deduceExprType(expr)); - auto& name = expr->prop(); - auto eq = [&](const ColDef& col) { return col.name == name; }; - auto iter = std::find_if(outputs_.cbegin(), outputs_.cend(), eq); - size_t colIdx = std::distance(outputs_.cbegin(), iter); - colOrderTypes_.emplace_back(std::make_pair(colIdx, factor->orderType())); + } + + + if (!exprProps_.inputProps().empty() && !exprProps_.varProps().empty()) { + return Status::SemanticError("Not support both input and variable."); + } else if (!exprProps_.inputProps().empty()) { + outputs_ = inputCols(); + } else if (!exprProps_.varProps().empty()) { + if (!userDefinedVarNameList_.empty()) { + if (userDefinedVarNameList_.size() != 1) { + return Status::SemanticError("Multiple user defined vars are not supported yet."); + } + userDefinedVarName_ = *userDefinedVarNameList_.begin(); + outputs_ = vctx_->getVar(userDefinedVarName_); + } } return Status::OK(); @@ -46,6 +67,10 @@ Status OrderByValidator::toPlan() { colNames.emplace_back(col.name); } sortNode->setColNames(std::move(colNames)); + if (!userDefinedVarName_.empty()) { + sortNode->setInputVar(userDefinedVarName_); + } + root_ = sortNode; tail_ = root_; return Status::OK(); diff --git a/src/validator/OrderByValidator.h b/src/validator/OrderByValidator.h index 4f28c7017..8189af1f2 100644 --- a/src/validator/OrderByValidator.h +++ b/src/validator/OrderByValidator.h @@ -25,6 +25,7 @@ class OrderByValidator final : public Validator { private: std::vector> colOrderTypes_; + std::string userDefinedVarName_; }; } // namespace graph } // namespace nebula diff --git a/src/validator/SequentialValidator.cpp b/src/validator/SequentialValidator.cpp index 01483e400..9852f27a7 100644 --- a/src/validator/SequentialValidator.cpp +++ b/src/validator/SequentialValidator.cpp @@ -31,16 +31,6 @@ Status SequentialValidator::validateImpl() { } DCHECK(!sentences.empty()); - auto firstSentence = getFirstSentence(sentences.front()); - switch (firstSentence->kind()) { - case Sentence::Kind::kLimit: - case Sentence::Kind::kOrderBy: - case Sentence::Kind::kGroupBy: - return Status::SyntaxError("Could not start with the statement: %s", - firstSentence->toString().c_str()); - default: - break; - } seqAstCtx_->startNode = StartNode::make(seqAstCtx_->qctx); for (auto* sentence : sentences) { diff --git a/tests/tck/features/fetch/FetchVertices.intVid.feature b/tests/tck/features/fetch/FetchVertices.intVid.feature index 272a59a6c..f44018c1c 100644 --- a/tests/tck/features/fetch/FetchVertices.intVid.feature +++ b/tests/tck/features/fetch/FetchVertices.intVid.feature @@ -39,7 +39,7 @@ Feature: Fetch Int Vid Vertices """ $var = GO FROM hash('Boris Diaw') over like YIELD like._dst as id; FETCH PROP ON player $var.id YIELD player.name as name, player.age | - ORDER BY name + ORDER BY $-.name """ Then the result should be, in order, and the columns 0 should be hashed: | VertexID | name | player.age | diff --git a/tests/tck/features/fetch/FetchVertices.strVid.feature b/tests/tck/features/fetch/FetchVertices.strVid.feature index fc41a5bee..39330288d 100644 --- a/tests/tck/features/fetch/FetchVertices.strVid.feature +++ b/tests/tck/features/fetch/FetchVertices.strVid.feature @@ -47,7 +47,7 @@ Feature: Fetch String Vertices """ $var = GO FROM 'Boris Diaw' over like YIELD like._dst as id; FETCH PROP ON player $var.id YIELD player.name as name, player.age | - ORDER BY name + ORDER BY $-.name """ Then the result should be, in order: | VertexID | name | player.age | diff --git a/tests/tck/features/go/GroupbyLimit.feature b/tests/tck/features/go/GroupbyLimit.feature index 8e1c0b526..6c3624e23 100644 --- a/tests/tck/features/go/GroupbyLimit.feature +++ b/tests/tck/features/go/GroupbyLimit.feature @@ -212,6 +212,11 @@ Feature: Groupby & limit Sentence | "Grizzlies" | 34 | | "Raptors" | 34 | | "Lakers" | 40 | + When executing query: + """ + GROUP BY 1 YIELD 1 + """ + Then a SemanticError should be raised at runtime: Scenario: Groupby with all agg functions When executing query: diff --git a/tests/tck/features/go/Orderby.feature b/tests/tck/features/go/Orderby.feature index f31f53293..dee1730e2 100644 --- a/tests/tck/features/go/Orderby.feature +++ b/tests/tck/features/go/Orderby.feature @@ -7,7 +7,7 @@ Feature: Orderby Sentence Background: Prepare space Given a graph with space named "nba" - Scenario: Syntax Error + Scenario: Syntax Error or Semantic Error When executing query: """ ORDER BY @@ -22,7 +22,12 @@ Feature: Orderby Sentence """ ORDER BY $-.xx """ - Then a SyntaxError should be raised at runtime: + Then a SemanticError should be raised at runtime: `$-.xx', not exist prop `xx' + When executing query: + """ + ORDER BY 1 + """ + Then a SemanticError should be raised at runtime: Order by with invalid expression `1' Scenario: Empty Input When executing query: @@ -165,6 +170,41 @@ Feature: Orderby Sentence | "Spurs" | | "Hornets" | + Scenario: Order by with Variable + When executing query: + """ + $var = GO FROM "Tony Parker" OVER like YIELD like._dst AS dst; ORDER BY $var.dst DESC + """ + Then the result should be, in order, with relax comparison: + | dst | + | "Tim Duncan" | + | "Manu Ginobili" | + | "LaMarcus Aldridge" | + When executing query: + """ + $var = GO FROM "Tony Parker" OVER like YIELD like._dst AS dst, like.likeness AS likeness; ORDER BY $var.dst DESC, $var.likeness + """ + Then the result should be, in order, with relax comparison: + | dst | likeness | + | "Tim Duncan" | 95 | + | "Manu Ginobili" | 95 | + | "LaMarcus Aldridge" | 90 | + When executing query: + """ + $var = GO FROM "Tony Parker" OVER like YIELD like._dst AS dst; ORDER BY $var.dst DESC | FETCH PROP ON * $-.dst + """ + Then the result should be, in order, with relax comparison: + | vertices_ | + | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) | + | ("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"}) | + | ("LaMarcus Aldridge" :player{age: 33, name: "LaMarcus Aldridge"}) | + When executing query: + """ + $var = GO FROM "Tony Parker" OVER like YIELD like._dst AS dst; + GO FROM $var.dst OVER like YIELD like._dst AS id | ORDER BY $var.dst, $-.id; + """ + Then a SemanticError should be raised at runtime: Not support both input and variable. + Scenario: Duplicate columns When executing query: """ diff --git a/tests/tck/features/lookup/ByIndex.feature b/tests/tck/features/lookup/ByIndex.feature index c1c09165f..da4c8e213 100644 --- a/tests/tck/features/lookup/ByIndex.feature +++ b/tests/tck/features/lookup/ByIndex.feature @@ -528,7 +528,7 @@ Feature: Lookup by index itself When executing query: """ LOOKUP ON player WHERE player.age < 40 - YIELD player.age AS Age, player.name AS Name | order by Age DESC, Name| limit 10 + YIELD player.age AS Age, player.name AS Name | order by $-.Age DESC, $-.Name| limit 10 """ Then the result should be, in order, with relax comparison: | VertexID | Age | Name | @@ -545,7 +545,7 @@ Feature: Lookup by index itself When executing query: """ LOOKUP ON player WHERE player.age <= 40 - YIELD player.age AS Age, player.name AS Name | order by Age DESC, Name| limit 10 + YIELD player.age AS Age, player.name AS Name | order by $-.Age DESC, $-.Name| limit 10 """ Then the result should be, in order, with relax comparison: | VertexID | Age | Name | diff --git a/tests/tck/features/lookup/ByIndex.intVid.feature b/tests/tck/features/lookup/ByIndex.intVid.feature index cc6ea594e..dfabfb19e 100644 --- a/tests/tck/features/lookup/ByIndex.intVid.feature +++ b/tests/tck/features/lookup/ByIndex.intVid.feature @@ -528,7 +528,7 @@ Feature: Lookup by index itself in integer vid When executing query: """ LOOKUP ON player WHERE player.age < 40 - YIELD player.age AS Age, player.name AS Name | order by Age DESC, Name| limit 10 + YIELD player.age AS Age, player.name AS Name | order by $-.Age DESC, $-.Name| limit 10 """ Then the result should be, in order, with relax comparison, and the columns 0 should be hashed: | VertexID | Age | Name | @@ -545,7 +545,7 @@ Feature: Lookup by index itself in integer vid When executing query: """ LOOKUP ON player WHERE player.age <= 40 - YIELD player.age AS Age, player.name AS Name | order by Age DESC, Name| limit 10 + YIELD player.age AS Age, player.name AS Name | order by $-.Age DESC, $-.Name| limit 10 """ Then the result should be, in order, with relax comparison, and the columns 0 should be hashed: | VertexID | Age | Name | diff --git a/tests/tck/features/optimizer/TopNRule.feature b/tests/tck/features/optimizer/TopNRule.feature index f040b79a2..ad2140e0f 100644 --- a/tests/tck/features/optimizer/TopNRule.feature +++ b/tests/tck/features/optimizer/TopNRule.feature @@ -12,7 +12,7 @@ Feature: TopN rule """ GO 1 STEPS FROM "Marco Belinelli" OVER like YIELD like.likeness AS likeness | - ORDER BY likeness | + ORDER BY $-.likeness | LIMIT 2 """ Then the result should be, in order: @@ -32,7 +32,7 @@ Feature: TopN rule """ GO 1 STEPS FROM "Marco Belinelli" OVER like REVERSELY YIELD like.likeness AS likeness | - ORDER BY likeness | + ORDER BY $-.likeness | LIMIT 1 """ Then the result should be, in order: @@ -51,7 +51,7 @@ Feature: TopN rule """ GO 1 STEPS FROM "Marco Belinelli" OVER like YIELD like.likeness as likeness | - ORDER BY likeness | + ORDER BY $-.likeness | LIMIT 2, 3 """ Then the result should be, in order: @@ -71,7 +71,7 @@ Feature: TopN rule """ GO 1 STEPS FROM "Marco Belinelli" OVER like YIELD like.likeness AS likeness | - ORDER BY likeness + ORDER BY $-.likeness """ Then the result should be, in any order: | likeness |