Support lookup indexScan using IN expression as filter #2906

Aiee · 2021-09-18T17:22:44Z

Fix constant fold for container expressions
Disable using other string-related relExpr as the filter of LOOKUP except STARTS WITH
Convert IN expression to OR expression in LOOKUP if the property in theIN expression has a valid index:
- Stand alone IN expr: A in [B, C, D] => (A == B) or (A == C) or (A == D)
- With AND expr : A in [a, b] AND B => (A==a AND B) OR (A==b AND B)
- With OR expr: A in [a, b] OR B => A==a OR A==b OR B
If the container has only one element, the IN expression will be transformed to a relEQ expression. e.g. A in [B] => A==B

tests/tck/features/lookup/EdgeIndexFullScan.feature

tests/tck/features/lookup/TagIndexFullScan.feature

czpmango · 2021-09-23T02:48:01Z

How do you evaluate the performance problems that this pr may bring, such as index degradation.
Or how to judge whether the or-index will always improve performance when the expression is complex.

czpmango · 2021-09-23T03:14:38Z

tests/tck/features/lookup/EdgeIndexFullScan.feature

-    When profiling query:
-      """
-      LOOKUP ON edge_1 WHERE edge_1.col1_str CONTAINS toLower("L") YIELD edge_1.col1_str
-      """


Delete these cases in another pr.

This is actually not deletion but modification, and this is to adapt the change made in LOOKUP validator. It makes more sense to me to include this in this PR.

czpmango · 2021-09-23T03:14:53Z

tests/tck/features/lookup/EdgeIndexFullScan.feature

      """
-      LOOKUP ON edge_1 WHERE edge_1.col1_str NOT CONTAINS toLower("L") YIELD edge_1.col1_str


src/graph/validator/LookupValidator.cpp

tests/tck/features/lookup/EdgeIndexFullScan.feature

Aiee · 2021-09-24T03:47:34Z

How do you evaluate the performance problems that this pr may bring, such as index degradation.
Or how to judge whether the or-index will always improve performance when the expression is complex.

This PR is based on an assumption, which is multiple prefix index scan is superior to a single full-index scan.
We can optimize this approach later, like limit the number of expanding IN expression to OR expression.

codecov-commenter · 2021-09-24T11:25:34Z

Codecov Report

Merging #2906 (34675db) into master (dddf54e) will increase coverage by 0.08%.
The diff coverage is 87.95%.

❗ Current head 34675db differs from pull request most recent head d05e508. Consider uploading reports for the commit d05e508 to get more accurate results

@@            Coverage Diff             @@
##           master    #2906      +/-   ##
==========================================
+ Coverage   85.47%   85.56%   +0.08%     
==========================================
  Files        1229     1229              
  Lines      110375   110651     +276     
==========================================
+ Hits        94341    94674     +333     
+ Misses      16034    15977      -57

Impacted Files	Coverage Δ
src/graph/util/ExpressionUtils.h	`100.00% <ø> (ø)`
src/graph/visitor/FindVisitor.h	`100.00% <ø> (ø)`
src/meta/processors/zone/ListZonesProcessor.cpp	`52.27% <23.80%> (-25.99%)`	⬇️
src/graph/validator/LookupValidator.cpp	`77.25% <81.39%> (+0.32%)`	⬆️
...ptimizer/rule/OptimizeTagIndexScanByFilterRule.cpp	`90.14% <85.00%> (-0.77%)`	⬇️
src/graph/util/ExpressionUtils.cpp	`92.92% <86.66%> (-0.99%)`	⬇️
...graph/optimizer/rule/UnionAllIndexScanBaseRule.cpp	`94.04% <93.75%> (+0.29%)`	⬆️
src/graph/optimizer/OptimizerUtils.cpp	`84.42% <100.00%> (+0.43%)`	⬆️
src/graph/util/test/ExpressionUtilsTest.cpp	`100.00% <100.00%> (ø)`
src/graph/visitor/FindVisitor.cpp	`97.61% <100.00%> (+0.11%)`	⬆️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b28d77...d05e508. Read the comment docs.

tests/tck/features/lookup/LookupTag2.feature

src/graph/visitor/FoldConstantExprVisitor.cpp

src/graph/visitor/VidExtractVisitor.cpp

Fix compilation

…OOKUP except STARTS/NOT STARTS WITH

Shylock-Hg · 2021-09-29T10:49:28Z

tests/tck/features/lookup/EdgeIndexFullScan.feature

    When executing query:
      """
      LOOKUP ON edge_1 WHERE edge_1.col1_str ENDS WITH "ABC" YIELD edge_1.col1_str
      """
-    Then the result should be, in any order:
-      | SrcVID | DstVID | Ranking | edge_1.col1_str |
+    Then a SemanticError should be raised at runtime: Expression (edge_1.col1_str ENDS WITH "ABC") is not supported, please use full-text index as an optimal solution


For this, maybe we could schedule index scan and full-text search automatically. Of course not in current PR.

bright-starry-sky · 2021-09-30T01:48:51Z

src/graph/util/test/ExpressionUtilsTest.cpp

+  // (a OR b) AND (c OR d)  ->  (a AND c) OR (a AND d) OR (b AND c) OR (b AND d)
+  {
+    auto andExpr = LogicalExpression::makeAnd(pool, orExpr1, orExpr2);
+    auto transformedExpr = ExpressionUtils::rewriteLogicalAndToLogicalOr(andExpr);
+
+    std::vector<Expression *> orOperands = {
+        LogicalExpression::makeAnd(
+            pool, ConstantExpression::make(pool, 10), ConstantExpression::make(pool, "a")),
+        LogicalExpression::makeAnd(
+            pool, ConstantExpression::make(pool, 10), ConstantExpression::make(pool, "b")),
+        LogicalExpression::makeAnd(
+            pool, ConstantExpression::make(pool, 20), ConstantExpression::make(pool, "a")),
+        LogicalExpression::makeAnd(


Do we support expression as A and (B or C ) and D ?

ExpressionUtils::rewriteLogicalAndToLogicalOr supports the rewrite but this function is only called when the filter contains IN expr for now. We could integrate this into AND expr with OR expr operand later.

PROFILE lookup on player where player.name < "zzzzz" and (player.age in[30, 42]) and player.name >= "Tim Duncan" -----+-----------+--------------+----------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------- | id | name | dependencies | profiling data | operator info | -----+-----------+--------------+----------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------- | 3 | Project | 4 | ver: 0, rows: 2, execTime: 50us, totalTime: 56us | outputVar: [ | | | | | | { | | | | | | "colNames": [ | | | | | | "VertexID" | | | | | | ], | | | | | | "type": "DATASET", | | | | | | "name": "__Project_3" | | | | | | } | | | | | | ] | | | | | | inputVar: __Filter_2 | | | | | | columns: [ | | | | | | "COLUMN[0] AS VertexID" | | | | | | ] | -----+-----------+--------------+----------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------- | 4 | IndexScan | 0 | { | outputVar: [ | | | | | ver: 0, rows: 2, execTime: 0us, totalTime: 13897us | { | | | | | storage_detail: {DedupNode:8465(us),IndexOpuputNode:8258(us),IndexScanNode:7456(us),IndexVertexNode:7895(us),RelNode:16610(us),} | "colNames": [ | | | | | "127.0.0.1":39535 exec/total: 8819(us)/13045(us) | "_vid", | | | | | } | "player.age", | | | | | | "player.name" | | | | | | ], | | | | | | "type": "DATASET", | | | | | | "name": "__Filter_2" | | | | | | } | | | | | | ] | | | | | | inputVar: | | | | | | space: 1 | | | | | | dedup: false | | | | | | limit: 9223372036854775807 | | | | | | filter: | | | | | | orderBy: [] | | | | | | schemaId: 2 | | | | | | isEdge: false | | | | | | returnCols: [ | | | | | | "_vid", | | | | | | "age", | | | | | | "name" | | | | | | ] | | | | | | indexCtx: [ | | | | | | { | | | | | | "columnHints": [ | | | | | | { | | | | | | "endValue": "__EMPTY__", | | | | | | "beginValue": "30", | | | | | | "scanType": "PREFIX", | | | | | | "column": "age" | | | | | | } | | | | | | ], | | | | | | "index_id": 25, | | | | | | "filter": "((player.age==30) AND (player.name<\"zzzzz\") AND (player.name>=\"Tim Duncan\"))" | | | | | | }, | | | | | | { | | | | | | "columnHints": [ | | | | | | { | | | | | | "beginValue": "42", | | | | | | "endValue": "__EMPTY__", | | | | | | "scanType": "PREFIX", | | | | | | "column": "age" | | | | | | } | | | | | | ], | | | | | | "index_id": 25, | | | | | | "filter": "((player.age==42) AND (player.name<\"zzzzz\") AND (player.name>=\"Tim Duncan\"))" | | | | | | } | | | | | | ] | -----+-----------+--------------+----------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------- | 0 | Start | | ver: 0, rows: 0, execTime: 2us, totalTime: 18us | outputVar: [ | | | | | | { | | | | | | "colNames": [], | | | | | | "type": "DATASET", | | | | | | "name": "__Start_0" | | | | | | } | | | | | | ] | -----+-----------+--------------+----------------------------------------------------------------------------------------------------------------------------------+---------------------------------------------------------------------------------------------------

bright-starry-sky · 2021-09-30T01:57:41Z

src/graph/util/ExpressionUtils.cpp

+  return orExpr;
+}
+
+Expression *ExpressionUtils::rewriteLogicalAndToLogicalOr(const Expression *expr) {


Why can't we use this method?
ExpressionUtils::expandExpr

Oops. I missed it. Seems ExpressionUtils::expandExpr has the functionality but is implemented in a recursive way. My consideration is that this may cause stack overflow when the number of elements in the list of IN expr becomes large.
Also, I checked the code base and did see this function was called anywhere. Could we consider removing it just for clarity?

Oops. I missed it. Seems ExpressionUtils::expandExpr has the functionality but is implemented in a recursive way. My consideration is that this may cause stack overflow when the number of elements in the list of IN expr becomes large. Also, I checked the code base and did see this function was called anywhere. Could we consider removing it just for clarity?

I agree to use your logic, which can be optimized in the next PR, but don't delete the existing methods.WDYT?

Could you list some usages of ExpressionUtils::expandExpr and explain a bit why this function is necessary?

bright-starry-sky · 2021-09-30T03:57:19Z

src/graph/optimizer/rule/OptimizeTagIndexScanByFilterRule.cpp

+  if (condition->kind() == ExprKind::kLogicalAnd) {
+    for (auto& operand : static_cast<const LogicalExpression*>(condition)->operands()) {
+      if (operand->kind() == ExprKind::kRelIn) {
+        auto inExpr = static_cast<RelationalExpression*>(operand);
+        // Do not apply this rule if the IN expr has a valid index or it has only 1 element in the
+        // list
+        if (static_cast<ListExpression*>(inExpr->right())->size() > 1) {
+          return TransformResult::noTransform();
+        } else {
+          transformedExpr = graph::ExpressionUtils::rewriteInExpr(condition);
+        }
+        if (OptimizerUtils::relExprHasIndex(inExpr, indexItems)) {
+          return TransformResult::noTransform();
+        }
+      }
+    }
+  }
+


IN this case, we can't determine that this expression is introduced by IN, for example:
where in (....) ---> correct
where a AND (b OR c) AND ... ---> incorrect

This is true.
This rule is to match AND expr containing IN expr as operand. i.g. a AND (b IN [1,2,3])

Actually, I don't really want to expand all AND expr to OR expr, since we don't know whether the property in the OR expression has a valid index or not unless we iterate all its operands. However, for IN expr a IN [b,c,d,....], we only need to check the index of the left operand a.

bright-starry-sky · 2021-09-30T04:10:47Z

src/graph/optimizer/OptimizerUtils.cpp

+bool OptimizerUtils::relExprHasIndex(
+    const Expression* expr,
+    const std::vector<std::shared_ptr<nebula::meta::cpp2::IndexItem>>& indexItems) {
+  DCHECK(expr->isRelExpr());
+
+  for (auto& index : indexItems) {
+    const auto& fields = index->get_fields();
+    if (fields.empty()) {
+      return false;
+    }
+
+    auto left = static_cast<const RelationalExpression*>(expr)->left();
+    DCHECK(left->kind() == Expression::Kind::kEdgeProperty ||
+           left->kind() == Expression::Kind::kTagProperty);
+
+    auto propExpr = static_cast<const PropertyExpression*>(left);
+    if (propExpr->prop() == fields[0].get_name()) {
+      return true;
+    }
+  }
+
+  return false;
+}
+


Please add test case as below:
tag t1 (c1, c2)
tag t2(c1, c2)
index i1 on t1 (c1, c2)
index i2 on t2 (c3, c4)
lookup on t2 where c1 in (.....) ----> this logic will return true if the guess is correct, Because index column name can be the same from different indexes.

Now we check the schema type in LookupValidator

(root@nebula) [nba]> lookup on player where team.name == "Tim Duncan" [ERROR (-1009)]: SemanticError: Schema name error: team

bright-starry-sky · 2021-09-30T04:22:07Z

src/graph/optimizer/rule/UnionAllIndexScanBaseRule.cpp

-    status = metaClient->getEdgeIndexesFromCache(scan->space());
-  }
+  auto status = node->kind() == graph::PlanNode::Kind::kTagIndexFullScan
+                    ? metaClient->getTagIndexesFromCache(scan->space())


What's about getTagIndexesByTagNameFromCache(spaceId, tagName) ?

Did you mean getTagIndexByNameFromCache()? Sounds like something we could optimize.
The current approach is:

get all indexItems in the space

check if indexItems is empty

OptimizerUtils::findOptimalIndex(operand, indexItems, &isPrefixScan, &ictx)

which can be turned into:

analyze the expression to get tag info

get indexItems usinggetTagIndexByNameFromCache(spaceID, tagName)

check if indexItems is empty

OptimizerUtils::findOptimalIndex(operand, indexItems, &isPrefixScan, &ictx)

I'll mark this as a TODO. WDYT?

Aiee added the ready-for-testing PR: ready for the CI test label Sep 18, 2021

Aiee mentioned this pull request Sep 18, 2021

Support lookup indexScan using IN expression as filter vesoft-inc/nebula-graph#1278

Closed

Aiee added the wip Solution: work in progress label Sep 18, 2021

Sophie-Xie requested review from yixinglu and jievince September 22, 2021 02:13

Sophie-Xie added this to the v2.6.0 milestone Sep 22, 2021

Aiee force-pushed the rewrite-in-expr branch 3 times, most recently from 33dc9be to 8a70911 Compare September 22, 2021 14:05

Aiee removed the wip Solution: work in progress label Sep 23, 2021

czpmango reviewed Sep 23, 2021

View reviewed changes

tests/tck/features/lookup/EdgeIndexFullScan.feature Show resolved Hide resolved

czpmango reviewed Sep 23, 2021

View reviewed changes

tests/tck/features/lookup/TagIndexFullScan.feature Show resolved Hide resolved

czpmango reviewed Sep 23, 2021

View reviewed changes

yixinglu reviewed Sep 23, 2021

View reviewed changes

src/graph/validator/LookupValidator.cpp Outdated Show resolved Hide resolved

tests/tck/features/lookup/EdgeIndexFullScan.feature Show resolved Hide resolved

Aiee force-pushed the rewrite-in-expr branch from 8a70911 to b296f9a Compare September 23, 2021 15:16

Aiee requested review from yixinglu and czpmango September 24, 2021 07:16

Aiee force-pushed the rewrite-in-expr branch from 4d17185 to d05e508 Compare September 26, 2021 07:58

yixinglu previously approved these changes Sep 28, 2021

View reviewed changes

yixinglu added incompatible PR: incompatible with the recently released version doc affected PR: improvements or additions to documentation and removed incompatible PR: incompatible with the recently released version labels Sep 28, 2021

Shylock-Hg reviewed Sep 29, 2021

View reviewed changes

tests/tck/features/lookup/LookupTag2.feature Show resolved Hide resolved

Aiee requested a review from Shylock-Hg September 29, 2021 05:55

Shylock-Hg reviewed Sep 29, 2021

View reviewed changes

src/graph/visitor/FoldConstantExprVisitor.cpp Show resolved Hide resolved

Shylock-Hg reviewed Sep 29, 2021

View reviewed changes

src/graph/visitor/VidExtractVisitor.cpp Show resolved Hide resolved

Aiee added 7 commits September 29, 2021 17:22

Move PR from nebula-graph

f0f7a88

Add method to convert AND expr to OR expr

2a6358e

Fix rewriteLogicalAndToLogicalOr() and clear tests

219f604

Fix tests

9d26461

Fix compilation

Forbid using string-related relational expressions as the filter of L…

0d18f13

…OOKUP except STARTS/NOT STARTS WITH

Address comments

959d7c6

Add uts for rewriteInExpr() and rewriteLogicalAndToLogicalOr()

38a2280

Aiee dismissed yixinglu’s stale review via 0d04710 September 29, 2021 09:22

Aiee force-pushed the rewrite-in-expr branch from f23b916 to 0d04710 Compare September 29, 2021 09:22

Check foldability of container expr in VidExtractVisitor

a8ce12a

Aiee force-pushed the rewrite-in-expr branch from 25ac033 to a8ce12a Compare September 29, 2021 09:26

Shylock-Hg reviewed Sep 29, 2021

View reviewed changes

Aiee requested a review from yixinglu September 29, 2021 11:38

yixinglu approved these changes Sep 29, 2021

View reviewed changes

bright-starry-sky reviewed Sep 30, 2021

View reviewed changes

czpmango approved these changes Sep 30, 2021

View reviewed changes

bright-starry-sky merged commit dd2a7d2 into vesoft-inc:master Sep 30, 2021

Aiee deleted the rewrite-in-expr branch September 30, 2021 09:35

jamieliu1023 mentioned this pull request Oct 2, 2021

Weekly Report 2021-10-02 vesoft-inc/nebula-community#28

Closed

Aiee mentioned this pull request Oct 13, 2021

Whether “not start with” should be supported？ #3044

Closed

jievince mentioned this pull request Oct 19, 2021

Geo spatial: 3. geography schema, data, index and optimization #3043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support lookup indexScan using IN expression as filter #2906

Support lookup indexScan using IN expression as filter #2906

Aiee commented Sep 18, 2021 •

edited

Loading

czpmango commented Sep 23, 2021

czpmango Sep 23, 2021

Aiee Sep 24, 2021

czpmango Sep 23, 2021

Aiee commented Sep 24, 2021

codecov-commenter commented Sep 24, 2021 •

edited

Loading

Shylock-Hg Sep 29, 2021

bright-starry-sky Sep 30, 2021

Aiee Sep 30, 2021

Aiee Sep 30, 2021

bright-starry-sky Sep 30, 2021

bright-starry-sky Sep 30, 2021

Aiee Sep 30, 2021 •

edited

Loading

bright-starry-sky Sep 30, 2021

Aiee Sep 30, 2021

bright-starry-sky Sep 30, 2021

Aiee Sep 30, 2021

bright-starry-sky Sep 30, 2021

Aiee Sep 30, 2021

bright-starry-sky Sep 30, 2021 •

edited

Loading

Aiee Sep 30, 2021

		"""
		LOOKUP ON edge_1 WHERE edge_1.col1_str NOT CONTAINS toLower("L") YIELD edge_1.col1_str

Support lookup indexScan using IN expression as filter #2906

Support lookup indexScan using IN expression as filter #2906

Conversation

Aiee commented Sep 18, 2021 • edited Loading

czpmango commented Sep 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aiee commented Sep 24, 2021

codecov-commenter commented Sep 24, 2021 • 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

Aiee Sep 30, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bright-starry-sky Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aiee commented Sep 18, 2021 •

edited

Loading

codecov-commenter commented Sep 24, 2021 •

edited

Loading

Aiee Sep 30, 2021 •

edited

Loading

bright-starry-sky Sep 30, 2021 •

edited

Loading