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 label tag expression toString function #5008

Merged
merged 12 commits into from
Dec 14, 2022
+123 −33
Merged

Fix label tag expression toString function #5008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
31b222b
Fix label tag property expression toString
yixinglu Dec 6, 2022
28dec9a
Fix LabelTagPropertyExpression toString
yixinglu Dec 6, 2022
2f513cf
Fix TCK
yixinglu Dec 6, 2022
cc5dd0a
Log query for opt error
yixinglu Dec 7, 2022
616ab65
Cleanup and line wrap
yixinglu Dec 8, 2022
c0fa68f
Fix tck tests
yixinglu Dec 8, 2022
12cd9fa
Merge branch 'master' into fix-label-tag-expr-tostring
yixinglu Dec 8, 2022
8e49bd2
Merge branch 'master' into fix-label-tag-expr-tostring
yixinglu Dec 8, 2022
8757886
Merge branch 'master' into fix-label-tag-expr-tostring
yixinglu Dec 9, 2022
9be5939
Merge branch 'master' into fix-label-tag-expr-tostring
yixinglu Dec 14, 2022
807d9f6
Merge branch 'master' into fix-label-tag-expr-tostring
jievince Dec 14, 2022
cff1d79
Merge branch 'master' into fix-label-tag-expr-tostring
yixinglu Dec 14, 2022
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
16 changes: 14 additions & 2 deletions src/common/expression/PropertyExpression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,20 @@ void LabelTagPropertyExpression::accept(ExprVisitor* visitor) {
}

std::string LabelTagPropertyExpression::toString() const {
std::string labelStr = label_ != nullptr ? label_->toString().erase(0, 1) : "";
return labelStr + "." + sym_ + "." + prop_;
std::string labelStr;
if (label_ != nullptr) {
labelStr = label_->toString();
// Remove the leading '$' character for variable except '$-/$$/$^'
if (labelStr.find(kInputRef) != 0 && labelStr.find(kSrcRef) != 0 &&
labelStr.find(kDstRef) != 0) {
labelStr.erase(0, 1);
}
labelStr += ".";
}
if (!sym_.empty()) {
labelStr += sym_ + ".";
Copy link
Contributor

@czpmango czpmango Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Better to use :

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like the following format?

$-:player.age
$$:player.name
v:player.name

}
return labelStr + prop_;
}

bool LabelTagPropertyExpression::operator==(const Expression& rhs) const {
Expand Down
3 changes: 2 additions & 1 deletion src/graph/service/QueryInstance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ Status QueryInstance::validateAndOptimize() {
// Optimize the query, and get the execution plan. We should not pass the optimizer errors to user
// since the message is often not easy to understand. Logging them is enough.
if (auto status = findBestPlan(); !status.ok()) {
LOG(ERROR) << "Error found in optimization stage: " << status.message();
LOG(ERROR) << "Error found in optimization stage for query: " << rctx->query()
<< ", error: " << status.message();
return Status::Error(
"There are some errors found in optimizer, "
"please contact to the admin to learn more details");
Comment on lines +96 to 100
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message on line 100 is not appropriate, it does not prevent the error message from being passed to the user, and it is more confusing than the original error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am not sure what's the better way to handle the optimizer errors. I need to open an issue about this and research how to do that in other db.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5033

Expand Down
25 changes: 22 additions & 3 deletions tests/tck/features/bugfix/AliasTypeDeduce.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,40 @@ Feature: Test extract filter
Scenario: Extract filter bug
When executing query:
"""
match p=allShortestPaths((v1)-[:like*1..3]-(v2)) where id(v1)=="Tim Duncan" and id(v2)=="Tony Parker" with nodes(p) as pathNodes WITH [n IN pathNodes | id(n)] AS personIdsInPath, [idx IN range(1, size(pathNodes)-1) | [prev IN [pathNodes[idx-1]] | [curr IN [pathNodes[idx]] | [prev, curr]]]] AS vertList UNWIND vertList AS c WITH c[0][0][0] AS prev , c[0][0][1] AS curr OPTIONAL MATCH (curr)<-[e:like]-(:player)-[:serve]->(:team)-[:teammate]->(prev) RETURN count(e) AS cnt1, prev, curr
match p=allShortestPaths((v1)-[:like*1..3]-(v2))
where id(v1)=="Tim Duncan" and id(v2)=="Tony Parker"
with nodes(p) as pathNodes
WITH
[n IN pathNodes | id(n)] AS personIdsInPath,
[idx IN range(1, size(pathNodes)-1) | [prev IN [pathNodes[idx-1]] | [curr IN [pathNodes[idx]] | [prev, curr]]]] AS vertList
UNWIND vertList AS c
WITH c[0][0][0] AS prev , c[0][0][1] AS curr
OPTIONAL MATCH (curr)<-[e:like]-(:player)-[:serve]->(:team)-[:teammate]->(prev)
RETURN count(e) AS cnt1, prev, curr
"""
Then the result should be, in any order:
| cnt1 | prev | curr |
| 0 | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) |
When executing query:
"""
match p=(a:player)-[e:like*1..3]->(b) where b.player.age>42 with relationships(p)[1] AS e1 match (b)-[:serve]->(c) where c.team.name>"S" and (b)-[e1]->() return count(c)
match p=(a:player)-[e:like*1..3]->(b)
where b.player.age>42
with relationships(p)[1] AS e1
match (b)-[:serve]->(c)
where c.team.name>"S" and (b)-[e1]->()
return count(c)
"""
Then the result should be, in any order:
| count(c) |
| 3225 |
When executing query:
"""
match p=(a:player)-[e:like*1..3]->(b) where b.player.age>42 with relationships(p)[1..2][0] AS e1 match (b)-[:serve]->(c) where c.team.name>"S" and (b)-[e1]->() return count(c)
match p=(a:player)-[e:like*1..3]->(b)
where b.player.age>42
with relationships(p)[1..2][0] AS e1
match (b)-[:serve]->(c)
where c.team.name>"S" and (b)-[e1]->()
return count(c)
"""
Then the result should be, in any order:
| count(c) |
Expand Down
23 changes: 12 additions & 11 deletions tests/tck/features/bugfix/PushFilterDownProject.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,21 @@ Feature: Test push filter down project
When profiling query:
"""
MATCH (n0)-[:like]->(n1)
WHERE (id(n0) IN ["Tim Duncan"])
WHERE id(n0) IN ['Tim Duncan']
WITH n1.player.age AS a0
WHERE ((a0 - (a0 + ((a0 % a0) + (a0 + a0)))) <= a0) RETURN count(*)
WHERE (a0 - (a0 + ((a0 % a0) + (a0 + a0)))) <= a0
RETURN count(*)
"""
Then the result should be, in any order:
| count(*) |
| 2 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Aggregate | 12 | |
| 12 | Project | 11 | |
| 11 | Filter | 5 | {"condition": "((-.n1.player.age-(-.n1.player.age+((-.n1.player.age%-.n1.player.age)+(-.n1.player.age+-.n1.player.age))))<=-.n1.player.age)"} |
| 5 | AppendVertices | 4 | |
| 4 | Traverse | 2 | |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
| id | name | dependencies | operator info |
| 9 | Aggregate | 12 | |
| 12 | Project | 11 | |
| 11 | Filter | 5 | {"condition": "(($-.n1.player.age-($-.n1.player.age+(($-.n1.player.age%$-.n1.player.age)+($-.n1.player.age+$-.n1.player.age))))<=$-.n1.player.age)"} |
| 5 | AppendVertices | 4 | |
| 4 | Traverse | 2 | |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
87 changes: 72 additions & 15 deletions tests/tck/features/optimizer/PushFilterDownHashInnerJoinRule.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,21 +149,78 @@ Feature: Push Filter down HashInnerJoin rule
| [:like "Tony Parker"->"Tim Duncan" @0 {likeness: 95}] | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) |
| [:like "Tim Duncan"->"Tony Parker" @0 {likeness: 95}] | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) |
And the execution plan should be:
| id | name | dependencies | operator info |
| 30 | Sort | 14 | |
| 14 | Project | 19 | |
| 19 | HashInnerJoin | 6,22 | |
| 6 | Project | 20 | |
| 20 | AppendVertices | 2 | |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
| 22 | Project | 21 | |
| 21 | Filter | 10 | { "condition": "(($-.e[0].likeness>0) OR (-.v1.player.age>0))" } |
| 10 | AppendVertices | 9 | |
| 9 | Traverse | 7 | |
| 7 | Argument | 8 | |
| 8 | Start | | |
| id | name | dependencies | operator info |
| 30 | Sort | 14 | |
| 14 | Project | 19 | |
| 19 | HashInnerJoin | 6,22 | |
| 6 | Project | 20 | |
| 20 | AppendVertices | 2 | |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
| 22 | Project | 21 | |
| 21 | Filter | 10 | { "condition": "(($-.e[0].likeness>0) OR ($-.v1.player.age>0))" } |
| 10 | AppendVertices | 9 | |
| 9 | Traverse | 7 | |
| 7 | Argument | 8 | |
| 8 | Start | | |
When profiling query:
"""
match (a:player)-[:like]->(b)
where b.player.age>30
match (b)-[:serve]->(c)
where c.team.name>'A' and b.player.age+a.player.age>40 and a.player.age<45
return a,b,c
order by a,b,c
limit 1
"""
Then the result should be, in any order:
| a | b | c |
| ("Amar'e Stoudemire" :player{age: 36, name: "Amar'e Stoudemire"}) | ("Steve Nash" :player{age: 45, name: "Steve Nash"}) | ("Lakers" :team{name: "Lakers"}) |
And the execution plan should be:
| id | name | dependencies | operator info |
| 16 | TopN | 25 | |
| 25 | HashInnerJoin | 27,29 | |
| 27 | Project | 30 | |
| 30 | Filter | 4 | {"condition": "((($-.b.player.age+$-.a.player.age)>40) AND ($-.a.player.age<45) AND ($-.b.player.age>30))"} |
| 4 | AppendVertices | 24 | |
| 24 | Traverse | 1 | |
| 1 | IndexScan | 2 | |
| 2 | Start | | |
| 29 | Project | 28 | |
| 28 | Filter | 9 | {"condition": "($-.c.team.name>\"A\")"} |
| 9 | AppendVertices | 8 | |
| 8 | Traverse | 7 | |
| 7 | Argument | | |
When profiling query:
"""
match (a:player)-[:like]->(b)
where b.player.age>30 or b.player.age>45
match (b)-[:serve]->(c)
where c.team.name>'A' or b.player.age+a.player.age>40 and a.player.age<45
return a,b,c
order by a,b,c
limit 1
"""
Then the result should be, in any order:
| a | b | c |
| ("Amar'e Stoudemire" :player{age: 36, name: "Amar'e Stoudemire"}) | ("Steve Nash" :player{age: 45, name: "Steve Nash"}) | ("Lakers" :team{name: "Lakers"}) |
And the execution plan should be:
| id | name | dependencies | operator info |
| 16 | TopN | 13 | |
| 13 | Project | 12 | |
| 12 | Filter | 11 | {"condition": "((c.team.name>\"A\") OR (((b.player.age+a.player.age)>40) AND (a.player.age<45)))" } |
| 11 | HashInnerJoin | 18,10 | |
| 18 | Project | 17 | |
| 17 | Filter | 4 | {"condition": "(($-.b.player.age>30) OR ($-.b.player.age>45))"} |
| 4 | AppendVertices | 19 | |
| 19 | Traverse | 1 | |
| 1 | IndexScan | 2 | |
| 2 | Start | | |
| 10 | Project | 9 | |
| 9 | AppendVertices | 8 | |
| 8 | Traverse | 7 | |
| 7 | Argument | | |

Scenario: NOT push filter down HashInnerJoin
When profiling query:
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/optimizer/PushFilterDownTraverseRule.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ Feature: Push Filter down Traverse rule
| id | name | dependencies | operator info |
| 11 | TopN | 10 | |
| 10 | Project | 9 | |
| 9 | Filter | 4 | {"condition": "(-.v.player.age!=35)" } |
| 9 | Filter | 4 | {"condition": "($-.v.player.age!=35)" } |
| 4 | AppendVertices | 12 | |
| 12 | Traverse | 8 | {"filter": "((like.likeness+100)!=199)"} |
| 8 | IndexScan | 2 | |
Expand Down