Fix rewrite edge node filter. #4815

Shylock-Hg · 2022-11-01T07:21:20Z

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/1534
Close #4989

Description:

Rewrite edge property like*.prop to _any(e1.prop, e2,prop ...)

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

codecov-commenter · 2022-11-07T04:05:56Z

Codecov Report

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

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4815      +/-   ##
==========================================
+ Coverage   76.78%   76.84%   +0.05%     
==========================================
  Files        1102     1102              
  Lines       80953    81001      +48     
==========================================
+ Hits        62160    62242      +82     
+ Misses      18793    18759      -34

Impacted Files	Coverage Δ
src/graph/validator/ACLValidator.h	`89.65% <ø> (ø)`
src/meta/processors/job/JobManager.h	`100.00% <ø> (ø)`
src/meta/processors/job/JobManager.cpp	`70.32% <6.25%> (-1.66%)`	⬇️
src/meta/processors/parts/DropSpaceProcessor.cpp	`64.35% <58.33%> (-0.82%)`	⬇️
src/storage/exec/StorageIterator.h	`93.33% <81.81%> (-1.98%)`	⬇️
src/common/function/FunctionManager.cpp	`79.44% <100.00%> (+0.12%)`	⬆️
src/graph/optimizer/rule/PushEFilterDownRule.cpp	`78.35% <100.00%> (-0.44%)`	⬇️
src/graph/validator/ACLValidator.cpp	`74.48% <100.00%> (+1.96%)`	⬆️
src/graph/visitor/ExtractFilterExprVisitor.cpp	`75.18% <100.00%> (+0.09%)`	⬆️
src/graph/visitor/PropertyTrackerVisitor.cpp	`82.97% <100.00%> (-1.28%)`	⬇️
... and 44 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.

yixinglu · 2022-11-07T06:20:44Z

tests/tck/features/bugfix/RewriteEdgeFilter.feature

+  Scenario: Order by after match
+    When executing query:
+      """
+      match (v)-[e:like|teammate{start_year: 2010}]->() where id(v) == 'Tim Duncan' return e


In openCypher, this edge filter means to find these edges which contain like or teammate labels and have the property start_year equal to 2010. So I think this filter should be converted to following format in NebulaGraph:

("like" in tags(e) and e.like.start_year = 2010) or ("teammate" in tags(e) and e.teammate.start_year=2010)

This is not only *.prop = xxxx, but we should have an equivalent representation of *.prop of edge property consists multiple edge types.

codesigner · 2022-11-07T06:53:17Z

src/common/function/FunctionManager.cpp

@@ -41,6 +42,7 @@ std::unordered_map<std::string, Value::Type> FunctionManager::variadicFunReturnT
    {"concat_ws", Value::Type::STRING},
    {"cos_similarity", Value::Type::FLOAT},
    {"coalesce", Value::Type::__EMPTY__},
+    {"_any", Value::Type::__EMPTY__},


the new added _any function, is it callable from query?

If I understood correctly, it seems to me that you mean _all, not _any. If there are 10 edge types, all of which has the same prop: prop_x, the semantics of any could be satisfied if only 1 such prop_x is found from all 10 edge types. all means making sure and finding all prop_x from all 10 edge types. I think you mean the latter one.

czpmango · 2022-11-08T02:42:36Z

src/graph/optimizer/rule/PushEFilterDownRule.cpp

      }
-      static_cast<LogicalExpression *>(ret)->setOperands(std::move(operands));
+      ret = FunctionCallExpression::make(pool, "_any", args);


Why not just rewrite it as like.start_year==2010 or teammate.start_year==2010?

How about others?

`like.start_year > xx' ?

`like.start_year > xx' ?

This filter could not be used in path pattern. I don't understand the purpose of the _any inner function.

Storage don't support *.prop, so rewrite it.

Agree with @czpmango comment.

I think what @Shylock-Hg wants is a generic way. In the path pattern alone, however, rewriting with == suffices.

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

src/common/function/FunctionManager.cpp

+          return arg.get();
+        }
+      }
+      return Value::kNullBadData;


kNullBadData is designed for error andkNullValue is sufficient for ignoring behavior. Fwiw, the bad null should be passed through by all functions or expressions.

What' your meaning?

xtcyclist

*.prop equals to something like union(e1.prop, e2,prop ...) semantically, not _any(e1.prop, e2,prop ...). Please make sure it's ok to change such semantics.

Shylock-Hg · 2022-12-02T02:33:22Z

*.prop equals to something like union(e1.prop, e2,prop ...) semantically, not _any(e1.prop, e2,prop ...). Please make sure it's ok to change such semantics.

match (v)-[e:like|serve]->() return e.likeness will return any non-null value of like.likeness or serve. likeness

jievince · 2022-12-05T13:37:57Z

Please look at this issue: #4989

jievince · 2022-12-06T02:40:28Z

src/common/function/FunctionManager.cpp

+  // Get any argument which is not empty/null
+  {
+    auto &attr = functions_["_any"];
+    attr.minArity_ = 1;
+    attr.maxArity_ = INT64_MAX;
+    attr.isAlwaysPure_ = true;
+    attr.body_ = [](const auto &args) -> Value {
+      for (const auto &arg : args) {
+        if (!arg.get().isNull() && !arg.get().empty()) {
+          return arg.get();
+        }
+      }
+      return Value::kNullValue;
+    };
+  }


I think _any is very similar to the function

coalesce() | coalesce(input :: ANY?) :: (ANY?) | Returns the first non-null value in a list of expressions.

Both neo4j and nebula support it.

Better to resuse it.

We should ignore EMPTY too.

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

Shylock-Hg marked this pull request as ready for review November 1, 2022 09:01

Shylock-Hg requested review from jievince, czpmango and Aiee November 1, 2022 09:01

Sophie-Xie added the ready for review label Nov 3, 2022

yixinglu reviewed Nov 7, 2022

View reviewed changes

Shylock-Hg requested a review from yixinglu November 7, 2022 06:31

codesigner reviewed Nov 7, 2022

View reviewed changes

czpmango reviewed Nov 8, 2022

View reviewed changes

Shylock-Hg requested review from codesigner and czpmango November 8, 2022 02:46

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 22, 2022 02:12

Shylock-Hg added 2 commits December 2, 2022 10:21

Fix rewrite edge node filter.

a259ff1

Fix return null.

55fa604

xtcyclist force-pushed the fix/rewrite-edge-filter branch from 44905d0 to 55fa604 Compare December 2, 2022 02:21

xtcyclist reviewed Dec 2, 2022

View reviewed changes

Shylock-Hg requested a review from xtcyclist December 2, 2022 02:34

xtcyclist approved these changes Dec 2, 2022

View reviewed changes

jievince reviewed Dec 6, 2022

View reviewed changes

Shylock-Hg requested a review from jievince December 6, 2022 03:30

jievince approved these changes Dec 6, 2022

View reviewed changes

Merge branch 'master' into fix/rewrite-edge-filter

43b60dc

jievince mentioned this pull request Dec 6, 2022

push filter down traverse rule #4987

Merged

11 tasks

yixinglu merged commit fef6560 into vesoft-inc:master Dec 6, 2022

Shylock-Hg deleted the fix/rewrite-edge-filter branch December 6, 2022 06:35

wey-gu mentioned this pull request Dec 10, 2022

Weekly Report 2022-12-09 vesoft-inc/nebula-community#146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rewrite edge node filter. #4815

Fix rewrite edge node filter. #4815

Shylock-Hg commented Nov 1, 2022 •

edited

Loading

codecov-commenter commented Nov 7, 2022 •

edited

Loading

yixinglu Nov 7, 2022

Shylock-Hg Nov 7, 2022

codesigner Nov 7, 2022

Shylock-Hg Nov 7, 2022

xtcyclist Dec 2, 2022

czpmango Nov 8, 2022

Shylock-Hg Nov 8, 2022

Shylock-Hg Nov 8, 2022

yixinglu Nov 17, 2022

Shylock-Hg Nov 17, 2022

yixinglu Nov 29, 2022

xtcyclist Dec 2, 2022

czpmango Nov 17, 2022

Shylock-Hg Nov 17, 2022

xtcyclist left a comment

Shylock-Hg commented Dec 2, 2022 •

edited

Loading

jievince commented Dec 5, 2022

jievince Dec 6, 2022 •

edited

Loading

Shylock-Hg Dec 6, 2022

jievince Dec 6, 2022

Shylock-Hg Dec 6, 2022

Fix rewrite edge node filter. #4815

Fix rewrite edge node filter. #4815

Conversation

Shylock-Hg commented Nov 1, 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:

codecov-commenter commented Nov 7, 2022 • edited Loading

Codecov Report

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

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 left a comment

Choose a reason for hiding this comment

Shylock-Hg commented Dec 2, 2022 • edited Loading

jievince commented Dec 5, 2022

jievince Dec 6, 2022 • edited Loading

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

Shylock-Hg commented Nov 1, 2022 •

edited

Loading

codecov-commenter commented Nov 7, 2022 •

edited

Loading

Shylock-Hg commented Dec 2, 2022 •

edited

Loading

jievince Dec 6, 2022 •

edited

Loading