Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct the yeild var input bug. #2789

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/graph/optimizer/rule/CollapseProjectRule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ StatusOr<OptRule::TransformResult> CollapseProjectRule::transform(
// 1. collect all property reference
std::vector<std::string> allPropRefNames;
for (auto col : colsAbove) {
if (graph::ExpressionUtils::findAny(
col->expr(), {Expression::Kind::kVar, Expression::Kind::kVersionedVar}) != nullptr) {
return TransformResult::noTransform();
}
std::vector<const Expression*> propRefs = graph::ExpressionUtils::collectAll(
col->expr(), {Expression::Kind::kVarProperty, Expression::Kind::kInputProperty});
for (auto* expr : propRefs) {
Expand Down
16 changes: 7 additions & 9 deletions src/graph/validator/YieldValidator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,20 @@ Status YieldValidator::validateImpl() {

if (!exprProps_.srcTagProps().empty() || !exprProps_.dstTagProps().empty() ||
!exprProps_.edgeProps().empty()) {
return Status::SemanticError("Only support input and variable in yield sentence.");
return Status::SemanticError("Not support vertex/edge property expression in yield sentence.");
}

if (!exprProps_.inputProps().empty() && !exprProps_.varProps().empty()) {
if (!exprProps_.inputProps().empty() && !userDefinedVarNameList_.empty()) {
return Status::SemanticError("Not support both input and variable.");
}

if (!exprProps_.varProps().empty() && exprProps_.varProps().size() > 1) {
return Status::SemanticError("Only one variable allowed to use.");
if (userDefinedVarNameList_.size() > 1) {
// Now disable yield multiple user defined input for that means implicit dataset join
// As the inner variable take care don't make it a dataset
return Status::SemanticError("Multiple variables not supported yet.");
}

if (!exprProps_.varProps().empty() && !userDefinedVarNameList_.empty()) {
// TODO: Support Multiple userDefinedVars
if (userDefinedVarNameList_.size() != 1) {
return Status::SemanticError("Multiple user defined vars not supported yet.");
}
if (!userDefinedVarNameList_.empty()) {
userDefinedVarName_ = *userDefinedVarNameList_.begin();
}

Expand Down
7 changes: 4 additions & 3 deletions src/graph/validator/test/YieldValidatorTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,8 @@ TEST_F(YieldValidatorTest, Error) {
// Not support reference two different variable
auto query = var + "YIELD $var.name WHERE $var1.start > 2005";
auto result = checkResult(query);
EXPECT_EQ(std::string(result.message()), "SemanticError: Only one variable allowed to use.");
EXPECT_EQ(std::string(result.message()),
"SemanticError: Multiple variables not supported yet.");
}
{
// Reference a non-existed prop is meaningless.
Expand All @@ -453,13 +454,13 @@ TEST_F(YieldValidatorTest, Error) {
auto query = var + "YIELD $$.person.name";
auto result = checkResult(query);
EXPECT_EQ(std::string(result.message()),
"SemanticError: Only support input and variable in yield sentence.");
"SemanticError: Not support vertex/edge property expression in yield sentence.");
}
{
auto query = var + "YIELD $^.person.name";
auto result = checkResult(query);
EXPECT_EQ(std::string(result.message()),
"SemanticError: Only support input and variable in yield sentence.");
"SemanticError: Not support vertex/edge property expression in yield sentence.");
}
{
auto query = var + "YIELD like.start";
Expand Down
12 changes: 10 additions & 2 deletions src/graph/visitor/DeducePropsVisitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,10 @@ void DeducePropsVisitor::visit(InputPropertyExpression *expr) {

void DeducePropsVisitor::visit(VariablePropertyExpression *expr) {
exprProps_->insertVarProp(expr->sym(), expr->prop());
userDefinedVarNameList_->emplace(expr->sym());
if (expr->sym()[0] != '_') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does if (expr->sym()[0] != '_') mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Means variable from query instead of inner generated variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better alternative implementation.
It's very unfriendly to someone reading this code.

// exclude inner generated variable
userDefinedVarNameList_->emplace(expr->sym());
}
}

void DeducePropsVisitor::visit(DestPropertyExpression *expr) {
Expand Down Expand Up @@ -168,7 +171,12 @@ void DeducePropsVisitor::visit(EdgeDstIdExpression *expr) { visitEdgePropExpr(ex

void DeducePropsVisitor::visit(UUIDExpression *expr) { reportError(expr); }

void DeducePropsVisitor::visit(VariableExpression *expr) { UNUSED(expr); }
void DeducePropsVisitor::visit(VariableExpression *expr) {
if (expr->var()[0] != '_') {
// exclude inner generated variable
userDefinedVarNameList_->emplace(expr->var());
}
}

void DeducePropsVisitor::visit(VersionedVariableExpression *expr) { reportError(expr); }

Expand Down
19 changes: 19 additions & 0 deletions tests/tck/features/bugfix/YieldVar.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Copyright (c) 2021 vesoft inc. All rights reserved.
#
# This source code is licensed under Apache 2.0 License,
# attached with Common Clause Condition 1.0, found in the LICENSES directory.
Feature: Test yield var

Background:
Given a graph with space named "nba"

# #2646
Scenario: yield constant after pipe
When executing query:
"""
$var = GO FROM "Tim Duncan" OVER like YIELD like._dst; YIELD $var[0][0] as var00;
"""
Then the result should be, in any order:
| var00 |
| "Manu Ginobili" |
| "Manu Ginobili" |
2 changes: 1 addition & 1 deletion tests/tck/features/yield/yield.IntVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ Feature: Yield Sentence
"""
$var = GO FROM hash("Boris Diaw") OVER serve YIELD $^.player.name AS name, serve.start_year AS start, $$.team.name AS team;YIELD $var.team WHERE $var1.start > 2005
"""
Then a SemanticError should be raised at runtime: Only one variable allowed to use.
Then a SemanticError should be raised at runtime: Multiple variables not supported yet.
When executing query:
"""
$var = GO FROM hash("Boris Diaw") OVER serve YIELD $^.player.name AS name, serve.start_year AS start, $$.team.name AS team;YIELD $var.abc
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/yield/yield.feature
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ Feature: Yield Sentence
"""
$var = GO FROM "Boris Diaw" OVER serve YIELD $^.player.name AS name, serve.start_year AS start, $$.team.name AS team;YIELD $var.team WHERE $var1.start > 2005
"""
Then a SemanticError should be raised at runtime: Only one variable allowed to use.
Then a SemanticError should be raised at runtime: Multiple variables not supported yet.
When executing query:
"""
$var = GO FROM "Boris Diaw" OVER serve YIELD $^.player.name AS name, serve.start_year AS start, $$.team.name AS team;YIELD $var.abc
Expand Down