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

Fix bug #1337 from ent #4740

Merged
merged 19 commits into from
Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from 6 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
15 changes: 14 additions & 1 deletion src/graph/validator/MatchValidator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,12 @@ Status MatchValidator::buildNodeInfo(const MatchPath *path,
auto alias = node->alias();
auto *props = node->props();
auto anonymous = false;
// if there exists some property with no tag, return a semantic error
if (node->labels() == nullptr && props != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly props != nullptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems ok to remove node->labels() == nullptr. Removed.

return Status::SemanticError("`%s:%s': No tag found for property.",
props->items()[0].first.c_str(),
props->items()[0].second->toString().c_str());
}
if (node->labels() != nullptr) {
auto &labels = node->labels()->labels();
for (const auto &label : labels) {
Expand All @@ -213,11 +219,18 @@ Status MatchValidator::buildNodeInfo(const MatchPath *path,
nodeAliases.emplace(alias, AliasType::kNode);
}
Expression *filter = nullptr;
/* Note(Xuntao): Commented out the following part. With the current parser,
if no tag is given in match clauses, node->props() is not nullptr but
node-labels() is. This is not supposed to be valid.
*/
/*
if (props != nullptr) {
auto result = makeNodeSubFilter(const_cast<MapExpression *>(props), "*");
NG_RETURN_IF_ERROR(result);
filter = result.value();
} else if (node->labels() != nullptr && !node->labels()->labels().empty()) {
} else
*/
if (node->labels() != nullptr && !node->labels()->labels().empty()) {
const auto &labels = node->labels()->labels();
for (const auto &label : labels) {
auto result = makeNodeSubFilter(label->props(), *label->label());
Expand Down
9 changes: 2 additions & 7 deletions tests/tck/features/match/Base.IntVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ Feature: Basic match
| "serve" | "Cavaliers" |
When executing query:
"""
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2 {name: "Cavaliers"})
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2:team{name: "Cavaliers"})
RETURN type(r) AS Type, v2.team.name AS Name
"""
Then the result should be, in any order:
Expand All @@ -137,7 +137,7 @@ Feature: Basic match
| "serve" | "Cavaliers" |
When executing query:
"""
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2 {name: "Cavaliers"})
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2:team{name: "Cavaliers"})
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI. Since all property names must be attached to a tag name, isn't this more intuitive to design the syntax:

MATCH (v1:player{player.name: "LeBron James"}) -[r:serve]-> (v2{team.name: "Cavaliers"})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. This seems like a request for a new alternative for writing match patterns. May need to discuss with PD (and check with GQL standards).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe open an issue on this?

WHERE r.start_year <= 2005 AND r.end_year >= 2005
RETURN r.start_year AS Start_Year, r.end_year AS Start_Year
"""
Expand Down Expand Up @@ -514,11 +514,6 @@ Feature: Basic match
MATCH (v) return v
"""
Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
When executing query:
"""
MATCH (v{name: "Tim Duncan"}) return v
"""
Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
When executing query:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not delete test cases. Modify it if the test case behavior changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Relocated this case to a new scenario match_with_wrong_syntax in Base.feature.

"""
MATCH (v:player:bachelor) RETURN v
Expand Down
9 changes: 2 additions & 7 deletions tests/tck/features/match/Base.feature
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ Feature: Basic match
| "serve" | "Cavaliers" |
When executing query:
"""
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2 {name: "Cavaliers"})
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2:team{name: "Cavaliers"})
RETURN type(r) AS Type, v2.team.name AS Name
"""
Then the result should be, in any order:
Expand All @@ -175,7 +175,7 @@ Feature: Basic match
| "serve" | "Cavaliers" |
When executing query:
"""
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2 {name: "Cavaliers"})
MATCH (v1:player{name: "LeBron James"}) -[r:serve]-> (v2:team{name: "Cavaliers"})
WHERE r.start_year <= 2005 AND r.end_year >= 2005
RETURN r.start_year AS Start_Year, r.end_year AS Start_Year
"""
Expand Down Expand Up @@ -624,11 +624,6 @@ Feature: Basic match
MATCH (v) return v
"""
Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
When executing query:
"""
MATCH (v{name: "Tim Duncan"}) return v
"""
Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
When executing query:
"""
MATCH (v:player:bachelor) RETURN v
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/match/MatchById.IntVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ Feature: Integer Vid Match By Id
| 'serve' | 'Lakers' |
When executing query:
"""
MATCH (v1) -[r:serve]-> (v2 {name: "Cavaliers"})
MATCH (v1) -[r:serve]-> (v2:team {name: "Cavaliers"})
WHERE id(v1) == hash("LeBron James")
RETURN type(r) AS Type, v2.team.name AS Name
"""
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/match/MatchById.feature
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ Feature: Match By Id
| 'serve' | 'Lakers' |
When executing query:
"""
MATCH (v1) -[r:serve]-> (v2 {name: "Cavaliers"})
MATCH (v1) -[r:serve]-> (v2:team{name: "Cavaliers"})
WHERE id(v1) == "LeBron James"
RETURN type(r) AS Type, v2.team.name AS Name
"""
Expand Down
17 changes: 17 additions & 0 deletions tests/tck/features/match/PathExpr.feature
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,23 @@ Feature: Basic match
Background:
Given a graph with space named "nba"

Scenario: Tagless property
When executing query:
"""
match p = (v{name: "hello"})-->(v1{name: "hello"}) where id(v) == "kk" return p limit 1;
"""
Then a SemanticError should be raised at runtime: `name:"hello"': No tag found for property.
When executing query:
"""
match p = (v:player{name: "hello"})-->(v1{name: "world"}) where id(v) == "kk" return p limit 1;
"""
Then a SemanticError should be raised at runtime: `name:"world"': No tag found for property.
When executing query:
"""
match p = (v{name: "hello"})-->(v1:player{name: "world"}) where id(v) == "kk" return p limit 1;
"""
Then a SemanticError should be raised at runtime: `name:"hello"': No tag found for property.

Scenario: Undefined aliases
When executing query:
"""
Expand Down
3 changes: 1 addition & 2 deletions tests/tck/features/match/Scan.feature
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,8 @@ Feature: Match seek by scan
Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
When executing query:
"""
MATCH (v{name: "Mary"})
MATCH (v:person)
RETURN v.student.name AS Name
LIMIT 3
"""
Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.

Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/match/VariableLengthPattern.feature
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ Feature: Variable length Pattern match (m to n)
Scenario: multi-steps and filter by node properties
When executing query:
"""
MATCH (v:player{name: 'Tim Duncan'})-[e1:like*1..2]-(v2{name: 'Tony Parker'})-[e2:serve]-(v3{name: 'Spurs'})
MATCH (v:player{name: 'Tim Duncan'})-[e1:like*1..2]-(v2:player{name: 'Tony Parker'})-[e2:serve]-(v3:team{name: 'Spurs'})
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix similar issue related to properties(v).tag.name, nodes(p)[0].tag.name, etc.

Copy link
Contributor Author

@xtcyclist xtcyclist Oct 24, 2022

Choose a reason for hiding this comment

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

If I understood what you mean correctly, all tck cases calling the 'properties' function runs as expected for the time being. I didn't find any abnormal case, at least. Let me know if any tck case needs to be updated.

If we want to force all properties returned by this function to report tag information, we probabaly need to check with PD and open a new issue to refactor this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these tck cases, I added tags into them, otherwise errors would be reported.

Copy link
Contributor

@czpmango czpmango Oct 24, 2022

Choose a reason for hiding this comment

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

Essentially, it's all the same issue, that is, the tag must be specified to get the property.
"all tck cases calling the 'properties' function runs as expected for the time being." just because the test case is not covered the exception scenario.

RETURN e1, e2
"""
Then the result should be, in any order, with relax comparison:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ Feature: Integer Vid Variable length Pattern match (m to n)
Scenario: Integer Vid multi-steps and filter by node properties
When executing query:
"""
MATCH (v:player{name: 'Tim Duncan'})-[e1:like*1..2]-(v2{name: 'Tony Parker'})-[e2:serve]-(v3{name: 'Spurs'})
MATCH (v:player{name: 'Tim Duncan'})-[e1:like*1..2]-(v2:player{name: 'Tony Parker'})-[e2:serve]-(v3:team{name: 'Spurs'})
RETURN e1, e2
"""
Then the result should be, in any order, with relax comparison:
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/yield/parameter.feature
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ Feature: Parameter
| "Tony Parker" |
When executing query:
"""
MATCH (v:player)-[:like]->(n{name:$p7.a.b.c})
MATCH (v:player)-[:like]->(n:player{name:$p7.a.b.c})
RETURN n.player.name AS dst LIMIT $p7.a.b.d[0]
"""
Then the result should be, in any order:
Expand Down