Fix extract filter. #4823

Shylock-Hg · 2022-11-04T02:06:25Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

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

Description:

It caused by extract filter don't reset ok status when the first logical and expr can be split.

How do you solve it?

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

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance 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 .....

czpmango · 2022-11-07T02:50:33Z

Please add some unit tests that can cover the modified behavior in ExtractFilterVisitorTest.cpp.

czpmango · 2022-11-07T03:20:45Z

I didn't reproduce the crash.

(root@nebula) [nba]> MATCH (v:player{name: 'Tim Duncan'})-[:like]->(t) WHERE ( (1 == 1 AND (is_edge(t))) OR (v.player.name == 'Tim Duncan')) RETURN v.player.name
+---------------+
| v.player.name |
+---------------+
| "Tim Duncan"  |
| "Tim Duncan"  |
+---------------+
Got 2 rows (time spent 7134/7445 us)

Shylock-Hg · 2022-11-07T03:24:11Z

I didn't reproduce the crash.

(root@nebula) [nba]> MATCH (v:player{name: 'Tim Duncan'})-[:like]->(t) WHERE ( (1 == 1 AND (is_edge(t))) OR (v.player.name == 'Tim Duncan')) RETURN v.player.name
+---------------+
| v.player.name |
+---------------+
| "Tim Duncan"  |
| "Tim Duncan"  |
+---------------+
Got 2 rows (time spent 7134/7445 us)

ent version

czpmango · 2022-11-07T03:42:26Z

I didn't reproduce the crash.

(root@nebula) [nba]> MATCH (v:player{name: 'Tim Duncan'})-[:like]->(t) WHERE ( (1 == 1 AND (is_edge(t))) OR (v.player.name == 'Tim Duncan')) RETURN v.player.name
+---------------+
| v.player.name |
+---------------+
| "Tim Duncan"  |
| "Tim Duncan"  |
+---------------+
Got 2 rows (time spent 7134/7445 us)

ent version

Can this issue be reproduced in the community edition? Bcz you are fixing a visitor rather than a rule that only the enterprise version has. For example, PushFilterDownGetNbrsRule also invoked this visitor.

Shylock-Hg · 2022-11-07T03:52:35Z

I didn't reproduce the crash.
(root@nebula) [nba]> MATCH (v:player{name: 'Tim Duncan'})-[:like]->(t) WHERE ( (1 == 1 AND (is_edge(t))) OR (v.player.name == 'Tim Duncan')) RETURN v.player.name
+---------------+
| v.player.name |
+---------------+
| "Tim Duncan"  |
| "Tim Duncan"  |
+---------------+
Got 2 rows (time spent 7134/7445 us)
ent version
Can this issue be reproduced in the community edition? Bcz you are fixing a visitor rather than a rule that only the enterprise version has. For example, PushFilterDownGetNbrsRule also invoked this visitor.

Not sure, I guess it will.

czpmango · 2022-11-07T06:46:56Z

src/graph/visitor/ExtractFilterExprVisitor.cpp

@@ -276,6 +276,7 @@ bool ExtractFilterExprVisitor::visitLogicalOr(LogicalExpression *expr) {
        if (canNotPushedIndex != -1 || !canBeSplit) {
          return false;
        }


Crashed at DCHECK(ok()) in ExprVisitorImpl.cpp:130.
Better to fix else branch:

else { if (!canBePushed_) { return false; } operands[i]->accept(this); }

Alternatively, we could do some minor refactoring of this visitor, such as adding additional state variables other than "can" and "cannot" pairs to allow the internal logic to continue.

Will lead to test fail.

This reverts commit 671f729.

…o fix/extract-filter

codecov-commenter · 2022-11-09T10:13:10Z

Codecov Report

Base: 76.78% // Head: 76.84% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (c99b2c9) compared to base (6f240e3).
Patch coverage: 35.48% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4823      +/-   ##
==========================================
+ Coverage   76.78%   76.84%   +0.06%     
==========================================
  Files        1102     1102              
  Lines       80953    80974      +21     
==========================================
+ Hits        62160    62225      +65     
+ Misses      18793    18749      -44

Impacted Files	Coverage Δ
src/meta/processors/job/JobManager.h	`100.00% <ø> (ø)`
src/meta/processors/job/JobManager.cpp	`70.01% <6.25%> (-1.97%)`	⬇️
src/meta/processors/parts/DropSpaceProcessor.cpp	`64.35% <58.33%> (-0.82%)`	⬇️
src/graph/visitor/ExtractFilterExprVisitor.cpp	`75.18% <100.00%> (+0.09%)`	⬆️
src/graph/visitor/PropertyTrackerVisitor.cpp	`82.97% <100.00%> (-1.28%)`	⬇️
src/meta/processors/job/ReportTaskProcessor.cpp	`85.71% <100.00%> (ø)`
...eta/processors/session/SessionManagerProcessor.cpp	`73.54% <0.00%> (-4.52%)`	⬇️
src/graph/executor/StorageAccessExecutor.h	`58.13% <0.00%> (-2.33%)`	⬇️
src/common/base/Status.h	`90.56% <0.00%> (-1.89%)`	⬇️
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

czpmango · 2022-11-17T03:38:46Z

src/graph/visitor/ExtractFilterExprVisitor.cpp

@@ -276,6 +276,7 @@ bool ExtractFilterExprVisitor::visitLogicalOr(LogicalExpression *expr) {
        if (canNotPushedIndex != -1 || !canBeSplit) {
          return false;
        }
+        canBePushed_ = true;  // reset when the LogicalAnd can be split


IMHO, this is not a good fix. Just fixed code path, but it's not a natural code logic.

That's only what I can do.

czpmango · 2022-11-17T03:39:39Z

tests/tck/features/bugfix/ExtractFilter.feature

+      """
+      MATCH (v:player{name: 'Tim Duncan'})-[:like]->(t) WHERE ( (1 == 1 AND (NOT is_edge(t))) OR (v.player.name == 'Tim Duncan')) RETURN v.player.name
+      """
+    Then the result should be, in any order:


Better to find the crash cases in community edition.

Can't do that.

czpmango

LGTM for now, but need to consider refactoring later.

Fix extract filter.

78c0cc3

Shylock-Hg requested review from czpmango, Aiee and jievince November 4, 2022 02:06

Shylock-Hg added the ready-for-testing PR: ready for the CI test label Nov 4, 2022

Merge branch 'master' into fix/extract-filter

e27c9c5

jievince previously approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'master' into fix/extract-filter

256fd91

czpmango reviewed Nov 7, 2022

View reviewed changes

Fail directly.

671f729

Shylock-Hg dismissed jievince’s stale review via 671f729 November 7, 2022 08:07

Merge branch 'master' into fix/extract-filter

a1054c3

Shylock-Hg requested review from czpmango and jievince November 7, 2022 08:10

Shylock-Hg added 2 commits November 7, 2022 17:11

Revert "Fail directly."

1e77e9e

This reverts commit 671f729.

Merge branch 'fix/extract-filter' of github.com:Shylock-Hg/nebula int…

8302156

…o fix/extract-filter

Shylock-Hg added the type/bug Type: something is unexpected label Nov 9, 2022

czpmango reviewed Nov 17, 2022

View reviewed changes

Shylock-Hg requested a review from czpmango November 18, 2022 02:45

czpmango approved these changes Nov 18, 2022

View reviewed changes

Merge branch 'master' into fix/extract-filter

c99b2c9

Aiee approved these changes Nov 18, 2022

View reviewed changes

Merge branch 'master' into fix/extract-filter

aec368f

Sophie-Xie merged commit 3e2d7b2 into vesoft-inc:master Nov 18, 2022

Shylock-Hg deleted the fix/extract-filter branch November 18, 2022 06:57

wey-gu mentioned this pull request Nov 19, 2022

Weekly Report 2022-11-18 vesoft-inc/nebula-community#143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix extract filter. #4823

Fix extract filter. #4823

Shylock-Hg commented Nov 4, 2022 •

edited

Loading

czpmango commented Nov 7, 2022 •

edited

Loading

czpmango commented Nov 7, 2022

Shylock-Hg commented Nov 7, 2022

czpmango commented Nov 7, 2022

Shylock-Hg commented Nov 7, 2022

czpmango Nov 7, 2022 •

edited

Loading

Shylock-Hg Nov 7, 2022

Shylock-Hg Nov 18, 2022

codecov-commenter commented Nov 9, 2022 •

edited

Loading

czpmango Nov 17, 2022

Shylock-Hg Nov 18, 2022

czpmango Nov 17, 2022

Shylock-Hg Nov 18, 2022

czpmango left a comment

Fix extract filter. #4823

Fix extract filter. #4823

Conversation

Shylock-Hg commented Nov 4, 2022 • edited 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:

czpmango commented Nov 7, 2022 • edited Loading

czpmango commented Nov 7, 2022

Shylock-Hg commented Nov 7, 2022

czpmango commented Nov 7, 2022

Shylock-Hg commented Nov 7, 2022

czpmango Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Shylock-Hg Nov 7, 2022

Choose a reason for hiding this comment

Shylock-Hg Nov 18, 2022

Choose a reason for hiding this comment

codecov-commenter commented Nov 9, 2022 • edited Loading

Codecov Report

czpmango Nov 17, 2022

Choose a reason for hiding this comment

Shylock-Hg Nov 18, 2022

Choose a reason for hiding this comment

czpmango Nov 17, 2022

Choose a reason for hiding this comment

Shylock-Hg Nov 18, 2022

Choose a reason for hiding this comment

czpmango left a comment

Choose a reason for hiding this comment

Shylock-Hg commented Nov 4, 2022 •

edited

Loading

czpmango commented Nov 7, 2022 •

edited

Loading

czpmango Nov 7, 2022 •

edited

Loading

codecov-commenter commented Nov 9, 2022 •

edited

Loading