Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Commit

Permalink
[cherry-pick #1222]fix the wrong usage of limit sentence (#1314)
Browse files Browse the repository at this point in the history
* fix the wrong usage of limit sentence

* add some tck tests

* support order by with variable

* address comments
  • Loading branch information
jievince authored Aug 11, 2021
1 parent 6a8570f commit aa5fbb4
Show file tree
Hide file tree
Showing 11 changed files with 99 additions and 38 deletions.
2 changes: 1 addition & 1 deletion src/parser/parser.yy
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand All @@ -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
Expand Down
55 changes: 40 additions & 15 deletions src/validator/OrderByValidator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,47 @@ namespace nebula {
namespace graph {
Status OrderByValidator::validateImpl() {
auto sentence = static_cast<OrderBySentence*>(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<const LabelExpression*>(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<InputPropertyExpression*>(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<VariablePropertyExpression*>(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<InputPropertyExpression*>(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();
Expand All @@ -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();
Expand Down
1 change: 1 addition & 0 deletions src/validator/OrderByValidator.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class OrderByValidator final : public Validator {

private:
std::vector<std::pair<size_t, OrderFactor::OrderType>> colOrderTypes_;
std::string userDefinedVarName_;
};
} // namespace graph
} // namespace nebula
Expand Down
10 changes: 0 additions & 10 deletions src/validator/SequentialValidator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/fetch/FetchVertices.intVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/fetch/FetchVertices.strVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
5 changes: 5 additions & 0 deletions tests/tck/features/go/GroupbyLimit.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
44 changes: 42 additions & 2 deletions tests/tck/features/go/Orderby.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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:
"""
Expand Down
4 changes: 2 additions & 2 deletions tests/tck/features/lookup/ByIndex.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand All @@ -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 |
Expand Down
4 changes: 2 additions & 2 deletions tests/tck/features/lookup/ByIndex.intVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand All @@ -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 |
Expand Down
8 changes: 4 additions & 4 deletions tests/tck/features/optimizer/TopNRule.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand All @@ -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:
Expand All @@ -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 |
Expand Down

0 comments on commit aa5fbb4

Please sign in to comment.