Fix expression rewrite #3614

Aiee · 2021-12-31T14:55:56Z

What type of PR is this?

bug
feature
enhancement

What problem does this PR solve?

Issue(s) number:
Fix #3248

Description:
This PR fixes filterTransform() and adds cases.

How do you slove it?

Do not transfer the filter expression if it contains 2 different tagProp expressions i.e. v.player.name =="abc" && v.player.age == 30
Fix expression overflow check. Do not rewrite the expression if it would cause an overflow.
Refactor rewriteRelExpr()
Return semantic error when the non-addition arithmetic expression contains strings.

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

(root@nebula) [nba]> match (v:player) where v.player.name+'n'=="Tim Duncann" return v
+-----------------------------------------------------+
| v                                                   |
+-----------------------------------------------------+
| ("Tim Duncan" :player{age: 42, name: "Tim Duncan"}) |
+-----------------------------------------------------+
Got 1 rows (time spent 5509/5861 us)

Fri, 31 Dec 2021 22:51:01 CST

(root@nebula) [NBA]> match (v:player) where v.player.name-'n'=="Tim Duncann" return v
`(v.player.name-"n")' is not a valid expression, can not apply `-' to `__EMPTY__' and `STRING'.

Fri, 31 Dec 2021 22:51:08 CST

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Perfomance test
N/A

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

Release notes:

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

ex. Fixed the bug .....

src/graph/util/ExpressionUtils.cpp

tests/tck/features/match/Base.feature

src/graph/util/ExpressionUtils.cpp

Shylock-Hg · 2022-01-07T09:15:47Z

conf/nebula-graphd.conf.default

@@ -84,8 +84,9 @@
 # System memory high watermark ratio, cancel the memory checking when the ratio greater than 1.0
 --system_memory_high_watermark_ratio=0.8

+########## metrics ##########


Better to do it in separate PR, don't modify unrelated code.

emm, just a format change

jievince · 2022-01-10T08:28:17Z

src/graph/util/ExpressionUtils.cpp

+    auto lExpr = const_cast<Expression *>(arithmExpr->left());
+    auto rExpr = const_cast<Expression *>(arithmExpr->right());


For constant expression, you could call its method value() directly, so that you don't need to call const_cast.

Here we are still not sure about the type of arithmExpr->left() and arithmExpr->right()

CPWstatic

Do we fix the bug that i meet in ldbc test cases?

        WHERE toInteger(message1.creationDate)/10000000000000   == year1
          AND toInteger(message1.creationDate)/100000000000%100 == month1

Aiee · 2022-01-11T03:35:19Z

Do we fix the bug that i meet in ldbc test cases?

        WHERE toInteger(message1.creationDate)/10000000000000   == year1
          AND toInteger(message1.creationDate)/100000000000%100 == month1

Yes the case you mentioned earlier has been fixed:

(root@nebula) [nba]> MATCH (v:player) WHERE toInteger(v.player.age)/10000000000000 == 10 AND toInteger(v.player.age)/10000000000000%100 == 1100000 RETURN v
+---+
| v |
+---+
+---+
Empty set (time spent 10065/10403 us)

CPWstatic

LGTM.

src/graph/util/ExpressionUtils.cpp

… exprs

Shylock-Hg · 2022-01-11T06:42:37Z

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

+      """
+      MATCH (v:player) WHERE v.player.name-'n'=="Tim Duncann" RETURN v
+      """
+    Then a SemanticError should be raised at runtime: `(v.player.name-"n")' is not a valid expression, can not apply `-' to `__EMPTY__' and `STRING'.


This error message is confusing. Where is the __EMPTY__?

The error message will try to evaluate the expression and print its type in the message. This is to unify the error messages we currently using.

src/graph/util/ExpressionUtils.cpp

* Do not transfer the filter expression if it contains 2 lableAttribute exprs * Fix expression overflow check * Fix rewriteRelExpr * Refactor rewriteRelExpr * Fix log usage * Check whether the minus expression contains string * Add tck cases * Address comments * Fix conflicts * Address comments * modify metrics in conf files * Address comments

* add LogMonitor to check log disk freeBytes and change log level when space is almost full (#3576) * add LogMonitor to check the log disk is full * fix comments * add comments for default flags Co-authored-by: yaphet <4414314+darionyaphet@users.noreply.github.com> * Ldbc test cases. (#3537) * Add ldbc test/ * Add all cases. * Fix some test cases. * Fix ldbc cases. * Fix pytest. Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com> * fix issue 3675 (#3678) * Fix expression rewrite (#3614) * Do not transfer the filter expression if it contains 2 lableAttribute exprs * Fix expression overflow check * Fix rewriteRelExpr * Refactor rewriteRelExpr * Fix log usage * Check whether the minus expression contains string * Add tck cases * Address comments * Fix conflicts * Address comments * modify metrics in conf files * Address comments * fix divide zone should be failed if two zones use the same host (#3699) * Support more check about merge zone arguments (#3703) * fix match index (#3694) * check scheam * add test case * fix graph crash * address comment' ' * add test case * fix error * fix vid select error * fix unit test error * address comment * fix issue 3601 (#3666) Co-authored-by: Nivras <12605142+Nivras@users.noreply.github.com> Co-authored-by: yaphet <4414314+darionyaphet@users.noreply.github.com> Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com> Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com> Co-authored-by: Yichen Wang <18348405+Aiee@users.noreply.github.com> Co-authored-by: jimingquan <mingquan.ji@vesoft.com> Co-authored-by: hs.zhang <22708345+cangfengzhs@users.noreply.github.com>

Aiee added ready-for-testing PR: ready for the CI test type/bug Type: something is unexpected wip Solution: work in progress and removed wip Solution: work in progress labels Dec 31, 2021

Sophie-Xie added the cherry-pick-v3.0 PR: need cherry-pick to this version label Jan 4, 2022

nevermore3 reviewed Jan 4, 2022

View reviewed changes

src/graph/util/ExpressionUtils.cpp Outdated Show resolved Hide resolved

src/graph/util/ExpressionUtils.cpp Outdated Show resolved Hide resolved

src/graph/util/ExpressionUtils.cpp Show resolved Hide resolved

Aiee force-pushed the fix-filter-expr branch from 9f86ed7 to 8942631 Compare January 4, 2022 08:21

Aiee requested review from Shylock-Hg and czpmango January 4, 2022 09:25

Aiee added the ready for review label Jan 4, 2022

Aiee force-pushed the fix-filter-expr branch 3 times, most recently from dc327b5 to 6b1f478 Compare January 6, 2022 04:46

Aiee requested a review from nevermore3 January 6, 2022 06:05

nevermore3 previously approved these changes Jan 6, 2022

View reviewed changes

Shylock-Hg reviewed Jan 6, 2022

View reviewed changes

tests/tck/features/match/Base.feature Outdated Show resolved Hide resolved

Shylock-Hg reviewed Jan 6, 2022

View reviewed changes

src/graph/util/ExpressionUtils.cpp Outdated Show resolved Hide resolved

Shylock-Hg reviewed Jan 6, 2022

View reviewed changes

src/graph/util/ExpressionUtils.cpp Show resolved Hide resolved

Aiee dismissed nevermore3’s stale review via 98ac865 January 7, 2022 04:45

Aiee force-pushed the fix-filter-expr branch from d39948b to 98ac865 Compare January 7, 2022 04:45

Aiee requested a review from Shylock-Hg January 7, 2022 04:48

Shylock-Hg reviewed Jan 7, 2022

View reviewed changes

jievince reviewed Jan 10, 2022

View reviewed changes

CPWstatic reviewed Jan 10, 2022

View reviewed changes

Aiee requested review from CPWstatic and jievince January 11, 2022 03:41

CPWstatic reviewed Jan 11, 2022

View reviewed changes

src/graph/util/ExpressionUtils.cpp Outdated Show resolved Hide resolved

src/graph/util/ExpressionUtils.cpp Outdated Show resolved Hide resolved

Aiee added 2 commits January 11, 2022 14:22

Do not transfer the filter expression if it contains 2 lableAttribute…

93ea748

… exprs

Fix expression overflow check

ad81c55

Aiee added 10 commits January 11, 2022 14:22

Fix rewriteRelExpr

cbf6f74

Refactor rewriteRelExpr

5d4081b

Fix log usage

9ca2b03

Check whether the minus expression contains string

cdcfea1

Add tck cases

34771df

Address comments

7389fff

Fix conflicts

7286943

Address comments

15142ea

modify metrics in conf files

2a9e88f

Address comments

a801264

Aiee force-pushed the fix-filter-expr branch from 3bf825d to a801264 Compare January 11, 2022 06:23

Aiee requested a review from CPWstatic January 11, 2022 06:24

CPWstatic approved these changes Jan 11, 2022

View reviewed changes

Shylock-Hg reviewed Jan 11, 2022

View reviewed changes

jievince approved these changes Jan 11, 2022

View reviewed changes

czpmango reviewed Jan 11, 2022

View reviewed changes

src/graph/util/ExpressionUtils.cpp Show resolved Hide resolved

czpmango reviewed Jan 11, 2022

View reviewed changes

src/graph/util/ExpressionUtils.cpp Show resolved Hide resolved

czpmango approved these changes Jan 11, 2022

View reviewed changes

Merge branch 'master' into fix-filter-expr

4ed1065

CPWstatic merged commit 32394f4 into vesoft-inc:master Jan 11, 2022

Aiee deleted the fix-filter-expr branch January 11, 2022 09:17

jamieliu1023 mentioned this pull request Jan 15, 2022

Weekly Report 2022-01-14 vesoft-inc/nebula-community#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix expression rewrite #3614

Fix expression rewrite #3614

Aiee commented Dec 31, 2021 •

edited

Loading

Shylock-Hg Jan 7, 2022

jievince Jan 10, 2022

jievince Jan 10, 2022

Aiee Jan 11, 2022

CPWstatic left a comment

Aiee commented Jan 11, 2022

CPWstatic left a comment

Shylock-Hg Jan 11, 2022

Aiee Jan 11, 2022

		auto lExpr = const_cast<Expression *>(arithmExpr->left());
		auto rExpr = const_cast<Expression *>(arithmExpr->right());

Fix expression rewrite #3614

Fix expression rewrite #3614

Conversation

Aiee commented Dec 31, 2021 • edited Loading

What type of PR is this?

What problem does this PR solve?

How do you slove it?

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

Checklist:

Release notes:

Shylock-Hg Jan 7, 2022

Choose a reason for hiding this comment

jievince Jan 10, 2022

Choose a reason for hiding this comment

jievince Jan 10, 2022

Choose a reason for hiding this comment

Aiee Jan 11, 2022

Choose a reason for hiding this comment

CPWstatic left a comment

Choose a reason for hiding this comment

Aiee commented Jan 11, 2022

CPWstatic left a comment

Choose a reason for hiding this comment

Shylock-Hg Jan 11, 2022

Choose a reason for hiding this comment

Aiee Jan 11, 2022

Choose a reason for hiding this comment

Aiee commented Dec 31, 2021 •

edited

Loading