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.

Sign up for GitHub

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

Fix pattern expression with same edge variable #5192

Merged
merged 3 commits into from
Jan 5, 2023
Merged

Fix pattern expression with same edge variable #5192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7e8397
Fix pattern expression with same edge variable
czpmango Jan 4, 2023
1730f97
add tck
czpmango Jan 5, 2023
3000a89
Merge branch 'master' into fix/pattern-expr-with-same-edge
czpmango Jan 5, 2023
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
9 changes: 9 additions & 0 deletions src/common/function/FunctionManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1867,6 +1867,15 @@ FunctionManager::FunctionManager() {
case Value::Type::EDGE: {
return value.getEdge().id();
}
// The root cause is the edge-type data format of Traverse executor
case Value::Type::LIST: {
auto &edges = value.getList().values;
if (edges.size() == 1 && edges[0].isEdge()) {
return edges[0].getEdge().id();
} else {
return args[0];
}
}
default: {
// Join on the origin type
return args[0];
Expand Down
4 changes: 2 additions & 2 deletions src/graph/planner/match/SegmentsConnector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ SubPlan SegmentsConnector::rollUpApply(CypherClauseContextBase* ctx,
std::vector<Expression*> compareProps;
for (const auto& col : path.compareVariables) {
compareProps.emplace_back(FunctionCallExpression::make(
qctx->objPool(), "id", {InputPropertyExpression::make(qctx->objPool(), col)}));
qctx->objPool(), "_joinkey", {InputPropertyExpression::make(qctx->objPool(), col)}));
}
InputPropertyExpression* collectProp = InputPropertyExpression::make(qctx->objPool(), collectCol);
auto* rollUpApply = RollUpApply::make(
Expand All @@ -104,7 +104,7 @@ SubPlan SegmentsConnector::rollUpApply(CypherClauseContextBase* ctx,
std::vector<Expression*> keyProps;
for (const auto& col : path.compareVariables) {
keyProps.emplace_back(FunctionCallExpression::make(
qctx->objPool(), "id", {InputPropertyExpression::make(qctx->objPool(), col)}));
qctx->objPool(), "_joinkey", {InputPropertyExpression::make(qctx->objPool(), col)}));
}
auto* patternApply = PatternApply::make(
qctx, left.root, DCHECK_NOTNULL(right.root), std::move(keyProps), path.isAntiPred);
Expand Down
19 changes: 15 additions & 4 deletions src/graph/validator/MatchValidator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,7 +1240,7 @@ Status MatchValidator::validatePathInWhere(
matchPath.alias()->c_str());
}
if (find->second != AliasType::kPath) {
return Status::SemanticError("Alias `%s' should be Path, but got type '%s",
return Status::SemanticError("`%s' is defined with type %s, but referenced with type Path",
matchPath.alias()->c_str(),
AliasTypeName::get(find->second).c_str());
}
Expand All @@ -1258,7 +1258,7 @@ Status MatchValidator::validatePathInWhere(
node->alias().c_str());
}
if (find->second != AliasType::kNode) {
return Status::SemanticError("Alias `%s' should be Node, but got type '%s",
return Status::SemanticError("`%s' is defined with type %s, but referenced with type Node",
node->alias().c_str(),
AliasTypeName::get(find->second).c_str());
}
Expand All @@ -1272,11 +1272,17 @@ Status MatchValidator::validatePathInWhere(
"PatternExpression are not allowed to introduce new variables: `%s'.",
edge->alias().c_str());
}
if (find->second != AliasType::kEdge) {
return Status::SemanticError("Alias `%s' should be Edge, but got type '%s'",
if (!edge->range() && find->second != AliasType::kEdge) {
return Status::SemanticError("`%s' is defined with type %s, but referenced with type Edge",
edge->alias().c_str(),
AliasTypeName::get(find->second).c_str());
}
if (edge->range() && find->second != AliasType::kEdgeList) {
return Status::SemanticError(
"`%s' is defined with type %s, but referenced with type EdgeList",
edge->alias().c_str(),
AliasTypeName::get(find->second).c_str());
}
}
}
return Status::OK();
Expand All @@ -1290,6 +1296,11 @@ Status MatchValidator::validatePathInWhere(
pathInfo.compareVariables.emplace_back(node->alias());
}
}
for (const auto &edge : path->edges()) {
if (edge->alias()[0] != '_') {
pathInfo.compareVariables.emplace_back(edge->alias());
}
}
pathInfo.collectVariable = *path->alias();
pathInfo.rollUpApply = true;
return Status::OK();
Expand Down
4 changes: 2 additions & 2 deletions tests/tck/features/bugfix/AliasTypeDeduce.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Feature: Test extract filter
"""
Then the result should be, in any order:
| count(c) |
| 3225 |
| 49 |
When executing query:
"""
match p=(a:player)-[e:like*1..3]->(b)
Expand All @@ -46,4 +46,4 @@ Feature: Test extract filter
"""
Then the result should be, in any order:
| count(c) |
| 3225 |
| 49 |
28 changes: 24 additions & 4 deletions tests/tck/features/match/PathExpr.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,42 @@ Feature: Basic match
"""
MATCH (v:player) WITH (v)-[v]-() AS p RETURN p
"""
Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous error message is a bit ambiguous.

Then a SemanticError should be raised at runtime: `v' is defined with type Node, but referenced with type Edge
When executing query:
"""
MATCH (v:player) UNWIND (v)-[v]-() AS p RETURN p
"""
Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node'
Then a SemanticError should be raised at runtime: `v' is defined with type Node, but referenced with type Edge
When executing query:
"""
MATCH (v:player) WHERE (v)-[v]-() RETURN v
"""
Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node'
Then a SemanticError should be raised at runtime: `v' is defined with type Node, but referenced with type Edge
When executing query:
"""
MATCH (v:player) RETURN (v)-[v]-()
"""
Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node'
Then a SemanticError should be raised at runtime: `v' is defined with type Node, but referenced with type Edge
When executing query:
"""
MATCH (v:player)-[e*3]->() RETURN (v)-[e]-()
"""
Then a SemanticError should be raised at runtime: `e' is defined with type EdgeList, but referenced with type Edge
When executing query:
"""
MATCH (v:player)-[e]->() RETURN (v)-[e*1..3]-()
"""
Then a SemanticError should be raised at runtime: `e' is defined with type Edge, but referenced with type EdgeList
When executing query:
"""
MATCH (v:player)-[e]->() RETURN (e)-[*1..3]-()
"""
Then a SemanticError should be raised at runtime: `e' is defined with type Edge, but referenced with type Node
When executing query:
"""
MATCH (v:player)-[e*3]->() RETURN (e)-[*1..3]-()
"""
Then a SemanticError should be raised at runtime: `e' is defined with type EdgeList, but referenced with type Node

Scenario: In Where
When executing query:
Expand Down
64 changes: 64 additions & 0 deletions tests/tck/features/match/PathExprRefLocalVariable.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,43 @@ Feature: Path expression reference local defined variables
| name |
| "Tim Duncan" |
| "Tim Duncan" |
When executing query:
czpmango marked this conversation as resolved.
Show resolved Hide resolved
"""
MATCH (v:player)-[e:like]->(n) WHERE (n)-[e]->(:player) RETURN v
"""
Then the result should be, in any order:
| v |
When executing query:
"""
MATCH (v:player)-[e]->(n) WHERE ()-[e]->(:player) and e.likeness<80 RETURN distinct v.player.name AS vname
"""
Then the result should be, in any order:
| vname |
| "Kyrie Irving" |
| "LaMarcus Aldridge" |
| "Dirk Nowitzki" |
| "Rudy Gay" |
| "Danny Green" |
| "Blake Griffin" |
| "Marco Belinelli" |
| "Vince Carter" |
| "Rajon Rondo" |
| "Ray Allen" |

@skip
Scenario: Fix after issue #2045
When executing query:
"""
MATCH (v:player)-[e:like*1..3]->(n) WHERE (n)-[e*1..4]->(:player) return v
"""
Then the result should be, in any order:
| v |
When executing query:
"""
MATCH (v:player)-[e:like*3]->(n) WHERE id(v)=="Tim Duncan" and (n)-[e*3]->(:player) return v
"""
Then the result should be, in any order:
| v |

Scenario: In With
When executing query:
Expand Down Expand Up @@ -182,6 +219,26 @@ Feature: Path expression reference local defined variables
| p |
| [[<("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"})-[:serve@0 {end_year: 2016, start_year: 1997}]->("Spurs" :team{name: "Spurs"})>]] |
| [[<("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"})-[:serve@0 {end_year: 2016, start_year: 1997}]->("Spurs" :team{name: "Spurs"})>]] |
When executing query:
"""
MATCH (v:player)-[e]->(n) WITH ()-[e{likeness:80}]->(:player) AS p1, ()-[e]-(:team) AS p2, v.player.name AS vname WHERE size(p1)>0 RETURN distinct * ORDER BY vname
"""
Then the result should be, in any order, with relax comparison:
| p1 | p2 | vname |
| [<("Aron Baynes" :player{age: 32, name: "Aron Baynes"})-[:like@0 {likeness: 80}]->("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"})>] | [] | "Aron Baynes" |
| [<("Ben Simmons" :player{age: 22, name: "Ben Simmons"})-[:like@0 {likeness: 80}]->("Joel Embiid" :player{age: 25, name: "Joel Embiid"})>] | [] | "Ben Simmons" |
| [<("Boris Diaw" :player{age: 36, name: "Boris Diaw"})-[:like@0 {likeness: 80}]->("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"})>] | [] | "Boris Diaw" |
| [<("Boris Diaw" :player{age: 36, name: "Boris Diaw"})-[:like@0 {likeness: 80}]->("Tony Parker" :player{age: 36, name: "Tony Parker"})>] | [] | "Boris Diaw" |
| [<("Damian Lillard" :player{age: 28, name: "Damian Lillard"})-[:like@0 {likeness: 80}]->("LaMarcus Aldridge" :player{age: 33, name: "LaMarcus Aldridge"})>] | [] | "Damian Lillard" |
| [<("Danny Green" :player{age: 31, name: "Danny Green"})-[:like@0 {likeness: 80}]->("LeBron James" :player{age: 34, name: "LeBron James"})>] | [] | "Danny Green" |
| [<("Dirk Nowitzki" :player{age: 40, name: "Dirk Nowitzki"})-[:like@0 {likeness: 80}]->("Steve Nash" :player{age: 45, name: "Steve Nash"})>] | [] | "Dirk Nowitzki" |
| [<("Dirk Nowitzki" :player{age: 40, name: "Dirk Nowitzki"})-[:like@0 {likeness: 80}]->("Jason Kidd" :player{age: 45, name: "Jason Kidd"})>] | [] | "Dirk Nowitzki" |
| [<("James Harden" :player{age: 29, name: "James Harden"})-[:like@0 {likeness: 80}]->("Russell Westbrook" :player{age: 30, name: "Russell Westbrook"})>] | [] | "James Harden" |
| [<("Jason Kidd" :player{age: 45, name: "Jason Kidd"})-[:like@0 {likeness: 80}]->("Vince Carter" :player{age: 42, name: "Vince Carter"})>] | [] | "Jason Kidd" |
| [<("Joel Embiid" :player{age: 25, name: "Joel Embiid"})-[:like@0 {likeness: 80}]->("Ben Simmons" :player{age: 22, name: "Ben Simmons"})>] | [] | "Joel Embiid" |
| [<("Luka Doncic" :player{age: 20, name: "Luka Doncic"})-[:like@0 {likeness: 80}]->("James Harden" :player{age: 29, name: "James Harden"})>] | [] | "Luka Doncic" |
| [<("Shaquille O'Neal" :player{age: 47, name: "Shaquille O'Neal"})-[:like@0 {likeness: 80}]->("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"})>] | [] | "Shaquille O'Neal" |
| [<("Tiago Splitter" :player{age: 34, name: "Tiago Splitter"})-[:like@0 {likeness: 80}]->("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"})>] | [] | "Tiago Splitter" |

Scenario: In Unwind
When executing query:
Expand All @@ -192,3 +249,10 @@ Feature: Path expression reference local defined variables
| p |
| [<("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"})-[:serve@0 {end_year: 2016, start_year: 1997}]->("Spurs" :team{name: "Spurs"})>] |
| [<("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"})-[:serve@0 {end_year: 2016, start_year: 1997}]->("Spurs" :team{name: "Spurs"})>] |
When executing query:
"""
MATCH (v:player{name: 'Tim Duncan'})-[e:like*1..3]->(n), (t:team {name: "Spurs"}) WITH v, e, collect(distinct n) AS ns UNWIND [n in ns | ()-[e*2..4]->(n:player)] AS p RETURN count(p) AS count
"""
Then the result should be, in any order:
| count |
| 11 |