lookup support push down topK #2992

MMyheart · 2021-09-30T07:47:39Z

No description provided.

Shylock-Hg · 2021-10-08T07:31:40Z

Add test cases.

bright-starry-sky · 2021-10-13T01:32:40Z

src/storage/exec/TopKNode.h

@@ -0,0 +1,125 @@
+/* Copyright (c) 2020 vesoft inc. All rights reserved.


bright-starry-sky · 2021-10-13T01:34:51Z

src/storage/exec/TopKNode.h

+  void adjustDown(size_t parent) {
+    size_t child = parent * 2 + 1;
+    size_t size = v_.size();
+    while (child < size) {
+      if (child + 1 < size && comparator_(v_[child], v_[child + 1])) {
+        child += 1;
+      }
+      if (!comparator_(v_[parent], v_[child])) {
+        return;
+      }
+      std::swap(v_[parent], v_[child]);
+      parent = child;
+      child = parent * 2 + 1;
+    }
+  }
+  void adjustUp(size_t child) {
+    size_t parent = (child - 1) >> 1;
+    while (0 != child) {
+      if (!comparator_(v_[parent], v_[child])) {
+        return;
+      }
+      std::swap(v_[parent], v_[child]);
+      child = parent;
+      parent = (child - 1) >> 1;
+    }
+  }


Not sure what this function means?
Is it any different from std::sort & std:: erase ？

I don't think it needs to sort all data by std::sort, and this function gets topK better than std::sort

bright-starry-sky · 2021-10-13T01:40:10Z

Good job. thanks for your contribution.
I have some ideas about this feature that we can discuss together.
For topK node, I think we'd better split it up into OrderNode and TopNode, they can be used for other queries , such as fetch and go statement. not just for lookup . WDYT?

bright-starry-sky · 2021-10-13T01:41:23Z

Good job. thanks for your contribution. I have some ideas about this feature that we can discuss together. For topK node, I think we'd better split it up into OrderNode and TopNode, they can be used for other queries , such as fetch and go statement. not just for lookup . WDYT?

And it is compatible with Order-by

MMyheart · 2021-10-14T08:30:29Z

Good job. thanks for your contribution. I have some ideas about this feature that we can discuss together. For topK node, I think we'd better split it up into OrderNode and TopNode, they can be used for other queries , such as fetch and go statement. not just for lookup . WDYT?

Firstly, I agree to that topK node also should be used for other queries, not just for lookup. And it can be implement by next pr. Secondly, I don't think it make sense to split topK up into OrderNode and TopNode. TopK Node is designed to solve order by xx | limit x. For only limit x, it should be part of other query node and for only order by xx, there is no need to push down.

Shylock-Hg · 2021-10-14T09:56:16Z

src/storage/exec/TopKNode.h

+namespace storage {
+
+template <class T>
+class TopKHeap final {


I think you could reuse the TokKHeap in graph layer, or merge it.

yeah, TopKHeap has been reused in IndexScanExecutor

MMyheart · 2021-10-18T09:00:39Z

Add test cases.

a test case has been added.
tests/tck/features/lookup/LookUpTopN.feature

Shylock-Hg · 2021-10-20T07:06:34Z

Please check the failed case.

Shylock-Hg · 2021-10-22T02:08:20Z

Please check the conflicts.

Shylock-Hg · 2021-10-22T07:38:00Z

tests/tck/features/lookup/LookUpTopN.feature

+      | 5  | DataCollect      | 9            |                           |
+      | 9  | Project          | 10           |                           |
+      | 10 | TopN             | 11           |                           |
+      | 11 | TagIndexFullScan | 0            | {"orderBy": {"pos": "0"}} |


I think you should check orderBy and limit fields of TagIndexFullScan`

I'm not sure what you mean

Check orderBy and limit together.

On line 45, there is limit check.
On line 27(here), there is orderBy check.
So, You can try to merge them together, thus , you can check orderBy and limit together.

src/interface/storage.thrift

src/graph/optimizer/rule/PushLimitSortDownIndexScanRule.h

src/graph/optimizer/rule/PushLimitDownProjectRule.cpp

codecov-commenter · 2021-10-27T08:07:32Z

Codecov Report

Merging #2992 (5bee437) into master (21a3ffb) will increase coverage by 0.09%.
The diff coverage is 90.03%.

@@            Coverage Diff             @@
##           master    #2992      +/-   ##
==========================================
+ Coverage   85.21%   85.31%   +0.09%     
==========================================
  Files        1295     1294       -1     
  Lines      118198   118308     +110     
==========================================
+ Hits       100728   100930     +202     
+ Misses      17470    17378      -92

Impacted Files	Coverage Δ
src/clients/storage/GraphStorageClient.h	`100.00% <ø> (ø)`
src/graph/executor/query/IndexScanExecutor.cpp	`91.66% <ø> (+2.77%)`	⬆️
src/graph/optimizer/rule/TopNRule.cpp	`95.45% <ø> (-0.38%)`	⬇️
src/storage/index/LookupBaseProcessor.h	`100.00% <ø> (ø)`
src/storage/index/LookupProcessor.cpp	`85.54% <50.00%> (-0.88%)`	⬇️
...rc/graph/optimizer/rule/PushLimitDownIndexRule.cpp	`80.00% <80.00%> (ø)`
src/graph/optimizer/rule/PushTopNDownIndexRule.cpp	`83.83% <83.83%> (ø)`
.../optimizer/rule/PushLimitSortDownIndexScanRule.cpp	`96.29% <96.29%> (ø)`
src/storage/exec/TopKNode.h	`96.29% <96.29%> (ø)`
src/clients/storage/GraphStorageClient.cpp	`76.43% <100.00%> (-0.11%)`	⬇️
... and 33 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 21a3ffb...5bee437. Read the comment docs.

Shylock-Hg · 2021-10-27T08:18:33Z

tests/tck/features/optimizer/TopNRule.feature

@@ -47,26 +47,6 @@ Feature: TopN rule
      | 4  | Start        |              |               |

  Scenario: [1] fail to apply topn rule
-    When profiling query:


Why delete this?

I think limit offset count can be pushed down, so I delete these. Is there a problem in some scenarios?

As I known, this pr just push topn to IndexScan, but the go statement implemented by GetNeighbors executor.

Shylock-Hg · 2021-10-27T09:54:33Z

tests/tck/features/go/GO.IntVid.feature

@@ -1639,21 +1639,21 @@ Feature: IntegerVid Go  Sentence
    When executing query:
      """
      $a = GO FROM hash('Tony Parker') OVER like YIELD like._src as src, like._dst as dst;
-      GO 2 STEPS FROM $a.src OVER like YIELD $a.src as src, $a.dst, like._src AS like_src, like._dst AS like_dst
-      | ORDER BY $-.src,$-.like_src,$-.like_dst | OFFSET 1 LIMIT 2
+      GO 2 STEPS FROM $a.src OVER like YIELD $a.src as src, $a.dst as dst, like._src AS like_src, like._dst AS like_dst


And why change tests of GO statement?

As above, this results in a change in the sorting results of two equal values.
eg:
The value list is 2、1、3、2' and the previous sorting result may be 1、2、2'、3, in other words, there's no change in the relative order of 2 and 2'. But now, it may be 1、2'、2、3, although the result has no effect on the user, but test case will be failed. So I change this.

jackwener · 2021-11-02T09:03:16Z

tests/tck/features/lookup/LookUpTopN.feature

+      | 0  | Start            |              |                           |
+    When profiling query:
+      """
+      LOOKUP ON player | ORDER BY $-.VertexID | Limit 5


As mentioned in the comments above, there is no need to test limit and orderBy check separately

Shylock-Hg · 2021-11-17T07:22:31Z

Hello @MMyheart :
The implementation of IndexScan had refactored by volcano model, so you should push the record to TopK node one by one.

Sophie-Xie · 2021-12-21T09:27:33Z

new PR #3499

czpmango requested review from Shylock-Hg and yixinglu September 30, 2021 07:56

HarrisChu added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Sep 30, 2021

MMyheart force-pushed the optimize/topN_push_down_in_lookup branch from 950dff9 to afa18ac Compare October 12, 2021 08:50

Sophie-Xie added the ready-for-testing PR: ready for the CI test label Oct 12, 2021

bright-starry-sky reviewed Oct 13, 2021

View reviewed changes

Sophie-Xie added the community Source: who proposed the issue label Oct 13, 2021

Shylock-Hg reviewed Oct 14, 2021

View reviewed changes

MMyheart force-pushed the optimize/topN_push_down_in_lookup branch 2 times, most recently from 393e20e to a76065c Compare October 18, 2021 08:55

Shylock-Hg requested review from Shylock-Hg, bright-starry-sky and jievince October 18, 2021 09:06

Shylock-Hg mentioned this pull request Oct 20, 2021

[Feature] order by | top push down #2948

Closed

MMyheart force-pushed the optimize/topN_push_down_in_lookup branch 3 times, most recently from 45c48d8 to 115434d Compare October 22, 2021 07:11

Shylock-Hg reviewed Oct 22, 2021

View reviewed changes

src/interface/storage.thrift Outdated Show resolved Hide resolved

Shylock-Hg reviewed Oct 22, 2021

View reviewed changes

src/graph/optimizer/rule/PushLimitSortDownIndexScanRule.h Outdated Show resolved Hide resolved

Shylock-Hg reviewed Oct 22, 2021

View reviewed changes

src/graph/optimizer/rule/PushLimitDownProjectRule.cpp Outdated Show resolved Hide resolved

MMyheart force-pushed the optimize/topN_push_down_in_lookup branch 2 times, most recently from af549ec to 27bdb7b Compare October 27, 2021 06:26

lookup support push down topK

5bee437

MMyheart force-pushed the optimize/topN_push_down_in_lookup branch from 27bdb7b to 5bee437 Compare October 27, 2021 06:35

Shylock-Hg reviewed Oct 27, 2021

View reviewed changes

jackwener reviewed Nov 2, 2021

View reviewed changes

Sophie-Xie requested a review from cangfengzhs November 19, 2021 01:22

Sophie-Xie closed this Dec 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lookup support push down topK #2992

lookup support push down topK #2992

MMyheart commented Sep 30, 2021

Shylock-Hg commented Oct 8, 2021

bright-starry-sky Oct 13, 2021

MMyheart Oct 18, 2021

bright-starry-sky Oct 13, 2021

MMyheart Oct 14, 2021 •

edited

Loading

bright-starry-sky commented Oct 13, 2021

bright-starry-sky commented Oct 13, 2021

MMyheart commented Oct 14, 2021

Shylock-Hg Oct 14, 2021

MMyheart Oct 18, 2021

MMyheart commented Oct 18, 2021

Shylock-Hg commented Oct 20, 2021

Shylock-Hg commented Oct 22, 2021

Shylock-Hg Oct 22, 2021

MMyheart Oct 22, 2021

Shylock-Hg Oct 25, 2021

jackwener Nov 2, 2021

codecov-commenter commented Oct 27, 2021

Shylock-Hg Oct 27, 2021

MMyheart Oct 27, 2021

Shylock-Hg Oct 27, 2021

Shylock-Hg Oct 27, 2021

MMyheart Oct 27, 2021

jackwener Nov 2, 2021

Shylock-Hg commented Nov 17, 2021 •

edited by Sophie-Xie

Loading

Sophie-Xie commented Dec 21, 2021

		@@ -0,0 +1,125 @@
		/* Copyright (c) 2020 vesoft inc. All rights reserved.

lookup support push down topK #2992

lookup support push down topK #2992

Conversation

MMyheart commented Sep 30, 2021

Shylock-Hg commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMyheart Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

bright-starry-sky commented Oct 13, 2021

bright-starry-sky commented Oct 13, 2021

MMyheart commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMyheart commented Oct 18, 2021

Shylock-Hg commented Oct 20, 2021

Shylock-Hg commented Oct 22, 2021

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

codecov-commenter commented Oct 27, 2021

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

Shylock-Hg commented Nov 17, 2021 • edited by Sophie-Xie Loading

Sophie-Xie commented Dec 21, 2021

MMyheart Oct 14, 2021 •

edited

Loading

Shylock-Hg commented Nov 17, 2021 •

edited by Sophie-Xie

Loading