Fix bug #1337 from ent #4740

xtcyclist · 2022-10-18T08:34:10Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

https://github.com/vesoft-inc/nebula-ent/issues/1337

(root@nebula) [nba]> create tag if not exists t1(name string);
Execution succeeded (time spent 557/783 us)

Mon, 26 Sep 2022 18:38:44 CST

(root@nebula) [nba]> create tag if not exists t2(name string);
Execution succeeded (time spent 564/798 us)

Mon, 26 Sep 2022 18:38:55 CST

(root@nebula) [nba]> insert vertex t1(name) values "kk":("hello");
Execution succeeded (time spent 701/932 us)

Mon, 26 Sep 2022 18:39:04 CST

(root@nebula) [nba]> insert vertex t2(name) values "kk":("world");
Execution succeeded (time spent 1113/1327 us)

Mon, 26 Sep 2022 18:39:11 CST

(root@nebula) [nba]> insert edge like(likeness) values "kk"->"kk"@1:(1)
Execution succeeded (time spent 1082/1288 us)

Mon, 26 Sep 2022 18:39:16 CST

(root@nebula) [nba]> match p = (v{name: "hello"})-->(v1{name: "hello"}) where id(v) == "kk" return p limit 1;
+----------------------------------------------------------------------------------------------------------------------+
| p                                                                                                                    |
+----------------------------------------------------------------------------------------------------------------------+
| <("kk" :t1{name: "hello"} :t2{name: "world"})-[:like@1 {likeness: 1}]->("kk" :t1{name: "hello"} :t2{name: "world"})> |
+----------------------------------------------------------------------------------------------------------------------+
Got 1 rows (time spent 2307/2652 us)

Mon, 26 Sep 2022 18:39:26 CST

(root@nebula) [nba]> match p = (v{name: "hello"})-->(v1{name: "world"}) where id(v) == "kk" return p limit 1;
+---+
| p |
+---+
+---+
Empty set (time spent 5376/6211 us)

Mon, 26 Sep 2022 18:39:31 CST

(root@nebula) [nba]>

Description:

Unexpected results returned by tag-less properties in the match clause.

How do you solve it?

All properties shall have tags. Return a semantic error message when there is none.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

In addition to report errors on tag-less properties, I also commented out the previous if branch that is responsible for makeNodeSubFilter() for tag-less properties.

I tested other clauses like lookup and go. The vertex.tag.property pattern seems good. Errors are reported if the tag part is ommitted.

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
TCK

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

The enterprise version.

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

…dating a match.

…t1337

Shylock-Hg · 2022-10-20T02:22:52Z

May need discuss with PD

Shylock-Hg · 2022-10-21T09:10:26Z

I think you should delete the syntax in parser too.

…dating a match.

xtcyclist · 2022-10-21T12:35:21Z

I think you should delete the syntax in parser too.

match_node
    : L_PAREN match_alias R_PAREN {
        $$ = new MatchNode(*$2, nullptr, nullptr);
        delete $2;
    }
    | L_PAREN match_alias match_node_label_list R_PAREN {
        $$ = new MatchNode(*$2, $3, nullptr);
        delete $2;
    }
//    | L_PAREN match_alias map_expression R_PAREN {
//        $$ = new MatchNode(*$2, nullptr, $3);
//        delete $2;
//    }
    ;

I tried. With the above syntax removed in parser.yy, the error message would become this:

(root@nebula) [nba]> match (v{name:"Boris Johson"}) return v limit 1;
[ERROR (-1004)]: SyntaxError: syntax error near `{name:"B'

I think it's better to report "No tag found for property". That would guide the user to add a tag to a specific property in the query. With only a syntax error, the user may be confused about what exaclty the error is.

@Shylock-Hg

Shylock-Hg · 2022-10-24T01:04:54Z

I think you should delete the syntax in parser too.
match_node
    : L_PAREN match_alias R_PAREN {
        $$ = new MatchNode(*$2, nullptr, nullptr);
        delete $2;
    }
    | L_PAREN match_alias match_node_label_list R_PAREN {
        $$ = new MatchNode(*$2, $3, nullptr);
        delete $2;
    }
//    | L_PAREN match_alias map_expression R_PAREN {
//        $$ = new MatchNode(*$2, nullptr, $3);
//        delete $2;
//    }
    ;
I tried. With the above syntax removed in parser.yy, the error message would become this:
(root@nebula) [nba]> match (v{name:"Boris Johson"}) return v limit 1;
[ERROR (-1004)]: SyntaxError: syntax error near `{name:"B'
I think it's better to report "No tag found for property". That would guide the user to add a tag to a specific property in the query. With only a syntax error, the user may be confused about what exaclty the error is.

@Shylock-Hg
OK.

Shylock-Hg · 2022-10-24T01:34:43Z

src/graph/validator/MatchValidator.cpp

@@ -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) {


Directly props != nullptr?

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

czpmango · 2022-10-24T02:46:48Z

tests/tck/features/match/Base.IntVid.feature

-      """
-      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:


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

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

…t1337

czpmango · 2022-10-24T03:07:24Z

tests/tck/features/match/Base.IntVid.feature

@@ -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"})


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"})

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).

Maybe open an issue on this?

czpmango · 2022-10-24T03:09:18Z

tests/tck/features/match/VariableLengthPattern.feature

@@ -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'})


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

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.

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

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 an semantic error if no tag is found for a property while validating a match. * Return an semantic error if no tag is found for a property while validating a match. * Add a tck case for the fixed bug. * commented out unused codes. * add tag in tck cases * fixing tck * updated tck cases to add missing cases that are supposed to be there. * Return an semantic error if no tag is found for a property while validating a match. * Add a tck case for the fixed bug. * commented out unused codes. * add tag in tck cases * fixing tck * updated tck cases to add missing cases that are supposed to be there. * update * update tck case. Co-authored-by: Sophie <[email protected]>

* Fix/find start error (#4771) * Fix find start error. * Fix test. Co-authored-by: Sophie <[email protected]> * Introduce JSON_EXTRACT function (#4743) * Introduce JSON_EXTRACT function close: #3513 Note, we don't support the path argument in this phase * address jievince's review commit removed the unecessary interface of construct Map from Value * Type handling Only primitive types are supported * Support depth1 nested * lint: fmt * ut: ctest, fixed wrong expression of Map * fix ut errors * tck: case for json_extract added Co-authored-by: Sophie <[email protected]> * Fix bug #1337 from ent (#4740) * Return an semantic error if no tag is found for a property while validating a match. * Return an semantic error if no tag is found for a property while validating a match. * Add a tck case for the fixed bug. * commented out unused codes. * add tag in tck cases * fixing tck * updated tck cases to add missing cases that are supposed to be there. * Return an semantic error if no tag is found for a property while validating a match. * Add a tck case for the fixed bug. * commented out unused codes. * add tag in tck cases * fixing tck * updated tck cases to add missing cases that are supposed to be there. * update * update tck case. Co-authored-by: Sophie <[email protected]> * Fix RollUpApplyExecutor (#4778) Co-authored-by: Sophie <[email protected]> * Fix mutil-match crash in optimization phase (#4780) fmt small fix small fix * fix subgraph step (#4776) * fix subgraph step * forbid function call in where clause * fix error Co-authored-by: shylock <[email protected]> Co-authored-by: Wey Gu <[email protected]> Co-authored-by: Cheng Xuntao <[email protected]> Co-authored-by: Yichen Wang <[email protected]> Co-authored-by: kyle.cao <[email protected]> Co-authored-by: jimingquan <[email protected]>

abby-cyber · 2023-01-18T07:46:34Z

vesoft-inc/nebula-docs-cn#2506

xtcyclist added 3 commits October 18, 2022 15:24

Return an semantic error if no tag is found for a property while vali…

1a824a4

…dating a match.

Return an semantic error if no tag is found for a property while vali…

cf82ca1

…dating a match.

Merge branch 'fix_ent1337' of github.com:xtcyclist/nebula into fix_en…

4de4900

…t1337

xtcyclist added the ready-for-testing PR: ready for the CI test label Oct 18, 2022

Add a tck case for the fixed bug.

dccb720

Sophie-Xie added the cherry-pick-v3.3 PR: need cherry-pick to this version label Oct 18, 2022

xtcyclist added 4 commits October 19, 2022 09:55

commented out unused codes.

67488ab

add tag in tck cases

b1b0576

fixing tck

099584e

updated tck cases to add missing cases that are supposed to be there.

8bec593

xtcyclist force-pushed the fix_ent1337 branch from 8bec593 to 0e5f0ff Compare October 19, 2022 07:57

xtcyclist requested review from Shylock-Hg, czpmango and nevermore3 October 19, 2022 08:36

xtcyclist added the ready for review label Oct 19, 2022

Sophie-Xie added this to the v3.3.0 milestone Oct 20, 2022

Sophie-Xie requested review from critical27 and MuYiYong October 20, 2022 05:32

critical27 previously approved these changes Oct 20, 2022

View reviewed changes

xtcyclist force-pushed the fix_ent1337 branch from 0e5f0ff to d37868c Compare October 20, 2022 06:04

Sophie-Xie dismissed critical27’s stale review via d37868c October 20, 2022 09:31

xtcyclist force-pushed the fix_ent1337 branch from d37868c to 4e569d7 Compare October 21, 2022 09:08

xtcyclist added 6 commits October 21, 2022 17:14

Return an semantic error if no tag is found for a property while vali…

4233b58

…dating a match.

Add a tck case for the fixed bug.

d7d0145

commented out unused codes.

eb24755

add tag in tck cases

ee22dc0

fixing tck

f9b59ae

updated tck cases to add missing cases that are supposed to be there.

5737b08

xtcyclist force-pushed the fix_ent1337 branch from 4e569d7 to 5737b08 Compare October 21, 2022 09:14

Shylock-Hg added the doc affected PR: improvements or additions to documentation label Oct 24, 2022

Shylock-Hg reviewed Oct 24, 2022

View reviewed changes

Merge branch 'master' into fix_ent1337

5d8a013

czpmango reviewed Oct 24, 2022

View reviewed changes

Merge branch 'fix_ent1337' of github.com:xtcyclist/nebula into fix_en…

da282af

…t1337

czpmango reviewed Oct 24, 2022

View reviewed changes

xtcyclist added 2 commits October 24, 2022 11:29

update

e7789ba

update tck case.

111b104

czpmango approved these changes Oct 24, 2022

View reviewed changes

Shylock-Hg approved these changes Oct 24, 2022

View reviewed changes

Merge branch 'master' into fix_ent1337

58f3363

Sophie-Xie merged commit 0d5d063 into vesoft-inc:master Oct 24, 2022

wey-gu mentioned this pull request Oct 29, 2022

Weekly Report 2022-10-28 vesoft-inc/nebula-community#140

Closed

Sophie-Xie mentioned this pull request Nov 10, 2022

unexpected match filter result #4853

Closed

abby-cyber self-assigned this Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug #1337 from ent #4740

Fix bug #1337 from ent #4740

xtcyclist commented Oct 18, 2022 •

edited by czpmango

Loading

Shylock-Hg commented Oct 20, 2022

Shylock-Hg commented Oct 21, 2022

xtcyclist commented Oct 21, 2022 •

edited

Loading

Shylock-Hg commented Oct 24, 2022

Shylock-Hg Oct 24, 2022

xtcyclist Oct 24, 2022

czpmango Oct 24, 2022

xtcyclist Oct 24, 2022

czpmango Oct 24, 2022

xtcyclist Oct 24, 2022

xtcyclist Oct 24, 2022

czpmango Oct 24, 2022

xtcyclist Oct 24, 2022 •

edited

Loading

xtcyclist Oct 24, 2022

czpmango Oct 24, 2022 •

edited

Loading

abby-cyber commented Jan 18, 2023

Fix bug #1337 from ent #4740

Fix bug #1337 from ent #4740

Conversation

xtcyclist commented Oct 18, 2022 • edited by czpmango Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Release notes:

Shylock-Hg commented Oct 20, 2022

Shylock-Hg commented Oct 21, 2022

xtcyclist commented Oct 21, 2022 • edited Loading

Shylock-Hg commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtcyclist Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czpmango Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

abby-cyber commented Jan 18, 2023

xtcyclist commented Oct 18, 2022 •

edited by czpmango

Loading

xtcyclist commented Oct 21, 2022 •

edited

Loading

xtcyclist Oct 24, 2022 •

edited

Loading

czpmango Oct 24, 2022 •

edited

Loading