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

Jump to bottom

Fix wrong properties prune in length function. #4884

Merged
merged 3 commits into from
Nov 18, 2022
Merged

Fix wrong properties prune in length function. #4884

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a773d69
Fix wrong properties prune in `length` function.
Shylock-Hg Nov 16, 2022
aae81c3
Merge branch 'master' into fix/wrong-properites-prune-length
Shylock-Hg Nov 16, 2022
9586537
Merge branch 'master' into fix/wrong-properites-prune-length
Sophie-Xie Nov 18, 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
4 changes: 3 additions & 1 deletion src/graph/visitor/PropertyTrackerVisitor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,10 @@ void PropertyTrackerVisitor::visit(AttributeExpression *expr) {
}

void PropertyTrackerVisitor::visit(FunctionCallExpression *expr) {
// length function support `STRING` input too, so we can't ignore it directly
// TODO add type info to variable to help optimize it.
static const std::unordered_set<std::string> ignoreFuncs = {
"src", "dst", "type", "typeid", "id", "rank", "length"};
"src", "dst", "type", "typeid", "id", "rank" /*, "length"*/};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we check if the argument type is path？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do it in later optimization. This PR just fix bug.

Copy link
Contributor

@jievince jievince Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I remember It was to optimze length(path) related query in ldbc. @nevermore3

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we can add the type of the variable to the operator later, which can be more precise when clipping attributes


auto funName = expr->name();
std::transform(funName.begin(), funName.end(), funName.begin(), ::tolower);
Expand Down
18 changes: 18 additions & 0 deletions tests/tck/features/bugfix/NullInputOfLength.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Copyright (c) 2022 vesoft inc. All rights reserved.
#
# This source code is licensed under Apache 2.0 License.
Feature: Null input of length function

Background:
Given a graph with space named "nba"

Scenario: Null input of length function
When executing query:
"""
match p = (v)-[:like]->() where id(v) == 'Tim Duncan'
return length(v.player.name) as l, length(p) as lp
"""
Then the result should be, in any order:
| l | lp |
| 10 | 1 |
| 10 | 1 |
44 changes: 22 additions & 22 deletions tests/tck/features/optimizer/PrunePropertiesRule.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,14 +439,14 @@ Feature: Prune Properties rule
| length(p) |
| 1 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 13 | Project | 11 | |
| 11 | Limit | 5 | |
| 5 | AppendVertices | 4 | { "props": "[{\"props\":[\"_tag\"],\"tagId\":8}, {\"props\":[\"_tag\"],\"tagId\":9}, {\"props\":[\"_tag\"],\"tagId\":10}]" } |
| 4 | Traverse | 2 | {"vertexProps": "", "edgeProps": "[{\"type\": 3, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
| id | name | dependencies | operator info |
| 13 | Project | 11 | |
| 11 | Limit | 5 | |
| 5 | AppendVertices | 4 | { "props": "[{\"props\":[\"_tag\", \"name\", \"speciality\"],\"tagId\":8}, {\"props\":[\"_tag\", \"name\", \"age\"],\"tagId\":9}, {\"props\":[\"_tag\", \"name\"],\"tagId\":10}]" } |
| 4 | Traverse | 2 | {"vertexProps": "[{\"props\":[\"name\", \"age\", \"_tag\"],\"tagId\":9}, {\"props\":[\"name\", \"speciality\", \"_tag\"],\"tagId\":8}, {\"props\":[\"name\", \"_tag\"],\"tagId\":10}]", "edgeProps": "[{\"type\": 3, \"props\": [\"_type\", \"_rank\", \"_dst\", \"_src\", \"likeness\"]}]" } |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
When profiling query:
"""
MATCH p = (a:player)-[e:like*1..3]->(b:player{age:39})
Expand All @@ -466,20 +466,20 @@ Feature: Prune Properties rule
| "Tracy McGrady" | 1 |
| "Tracy McGrady" | 3 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 14 | Project | 13 | |
| 13 | BiInnerJoin | 15,12 | |
| 15 | Project | 17 | |
| 17 | AppendVertices | 16 | { "props": "[{\"props\":[\"name\",\"age\"],\"tagId\":9}]" } |
| 16 | Traverse | 2 | {"vertexProps": "", "edgeProps": "[{\"type\": 3, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
| 12 | Project | 18 | |
| 18 | AppendVertices | 10 | { "props": "[{\"props\":[\"_tag\"],\"tagId\":10}]" } |
| 10 | Traverse | 8 | {"vertexProps": "[{\"props\":[\"name\",\"age\"],\"tagId\":9}]", "edgeProps": "[{\"type\": 4, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 8 | Argument | | |
| 9 | Start | | |
| id | name | dependencies | operator info |
| 14 | Project | 13 | |
| 13 | BiInnerJoin | 15,12 | |
| 15 | Project | 17 | |
| 17 | AppendVertices | 16 | { "props": "[{\"props\":[\"_tag\", \"name\", \"speciality\"],\"tagId\":8}, {\"props\":[\"_tag\", \"name\", \"age\"],\"tagId\":9}, {\"props\":[\"_tag\", \"name\"],\"tagId\":10}]" } |
| 16 | Traverse | 2 | {"vertexProps": "[{\"props\":[\"name\", \"age\", \"_tag\"],\"tagId\":9}, {\"props\":[\"name\", \"speciality\", \"_tag\"],\"tagId\":8}, {\"props\":[\"name\", \"_tag\"],\"tagId\":10}]", "edgeProps": "[{\"type\": 3, \"props\": [\"_type\", \"_rank\", \"_dst\", \"_src\", \"likeness\"]}]" } |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
| 12 | Project | 18 | |
| 18 | AppendVertices | 10 | { "props": "[{\"props\":[\"_tag\"],\"tagId\":10}]" } |
| 10 | Traverse | 8 | {"vertexProps": "[{\"props\":[\"name\", \"age\", \"_tag\"],\"tagId\":9}, {\"props\":[\"name\", \"speciality\", \"_tag\"],\"tagId\":8}, {\"props\":[\"name\", \"_tag\"],\"tagId\":10}]", "edgeProps": "[{\"type\": 4, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 8 | Argument | | |
| 9 | Start | | |

@distonly
Scenario: union match
Expand Down